Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discover and respect .python-version files in parent directories #6370

Open
wants to merge 1 commit into
base: tracking/050
Choose a base branch
from

Conversation

zanieb
Copy link
Member

@zanieb zanieb commented Aug 21, 2024

Uses #6369 for test coverage.

Updates version file discovery to search up into parent directories. Also refactors Python request determination to avoid duplicating the user request / version file / workspace lookup logic in every command (this supersedes the work started in #6372).

There is a bit of remaining work here, mostly around documentation. There are some edge-cases where we don't use the refactored request utility, like uv build — I'm not sure how I'm going to handle that yet as it needs a separate root directory.

Comment on lines 1076 to 1072
options
.stop_discovery_at
.and_then(Path::parent)
.map(|stop_discovery_at| stop_discovery_at != *path)
.unwrap_or(true)
})
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This code isn't used in production, but this updates the behavior in tandem with copying the implementation for PythonVersionFile::discover to solve a common footgun where you pass the project root as the place to stop discovery and discovery stopped before looking in that directory.

@zanieb zanieb marked this pull request as ready for review October 4, 2024 18:05
@zanieb zanieb added this to the v0.5.0 milestone Oct 4, 2024
Comment on lines 376 to 379
pub(crate) async fn from_request(
python_request: Option<PythonRequest>,
workspace: &Workspace,
workspace: Option<&Workspace>,
project_dir: &Path,
) -> Result<Self, ProjectError> {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of making workspace optional here, I might introduce a new abstraction which is used by WorkspacePython? It's doing a bit more work than it ought to as-is, but it's not clear another abstraction is worth it.

@zanieb zanieb mentioned this pull request Oct 24, 2024
@zanieb zanieb changed the base branch from main to tracking/050 October 24, 2024 16:56
} = ScriptPython::from_request(
python.as_deref().map(PythonRequest::parse),
None,
&Pep723Item::Script(script.clone()),
Copy link
Member Author

@zanieb zanieb Oct 24, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Annoyingly, I don't see a way to avoid this clone unless we make PEP723Item accept a borrowed script or create an into_script -> Option<...> method which we'd unwrap later. I think since it's in add it's fine to clone.

@zanieb
Copy link
Member Author

zanieb commented Oct 24, 2024

I'll do a self-review of this too. It'd been a minute since I wrote it...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking A breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant