-
Notifications
You must be signed in to change notification settings - Fork 3k
Refactor PythonVersionFile global loading
#14107
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -37,11 +37,14 @@ pub enum FilePreference { | |
| pub struct DiscoveryOptions<'a> { | ||
| /// The path to stop discovery at. | ||
| stop_discovery_at: Option<&'a Path>, | ||
| /// When `no_config` is set, Python version files will be ignored. | ||
| /// Ignore Python version files. | ||
| /// | ||
| /// Discovery will still run in order to display a log about the ignored file. | ||
| no_config: bool, | ||
| /// Whether `.python-version` or `.python-versions` should be preferred. | ||
| preference: FilePreference, | ||
| /// Whether to ignore local version files, and only search for a global one. | ||
| no_local: bool, | ||
| } | ||
|
|
||
| impl<'a> DiscoveryOptions<'a> { | ||
|
|
@@ -62,6 +65,11 @@ impl<'a> DiscoveryOptions<'a> { | |
| ..self | ||
| } | ||
| } | ||
|
|
||
| #[must_use] | ||
| pub fn with_no_local(self, no_local: bool) -> Self { | ||
| Self { no_local, ..self } | ||
| } | ||
| } | ||
|
|
||
| impl PythonVersionFile { | ||
|
|
@@ -70,33 +78,38 @@ impl PythonVersionFile { | |
| working_directory: impl AsRef<Path>, | ||
| options: &DiscoveryOptions<'_>, | ||
| ) -> Result<Option<Self>, std::io::Error> { | ||
| let Some(path) = Self::find_nearest(&working_directory, options) else { | ||
| if let Some(stop_discovery_at) = options.stop_discovery_at { | ||
| if stop_discovery_at == working_directory.as_ref() { | ||
| debug!( | ||
| "No Python version file found in workspace: {}", | ||
| working_directory.as_ref().display() | ||
| ); | ||
| let allow_local = !options.no_local; | ||
| let Some(path) = allow_local.then(|| { | ||
| // First, try to find a local version file. | ||
| let local = Self::find_nearest(&working_directory, options); | ||
| if local.is_none() { | ||
| // Log where we searched for the file, if not found | ||
| if let Some(stop_discovery_at) = options.stop_discovery_at { | ||
| if stop_discovery_at == working_directory.as_ref() { | ||
| debug!( | ||
| "No Python version file found in workspace: {}", | ||
| working_directory.as_ref().display() | ||
| ); | ||
| } else { | ||
| debug!( | ||
| "No Python version file found between working directory `{}` and workspace root `{}`", | ||
| working_directory.as_ref().display(), | ||
| stop_discovery_at.display() | ||
| ); | ||
| } | ||
| } else { | ||
| debug!( | ||
| "No Python version file found between working directory `{}` and workspace root `{}`", | ||
| working_directory.as_ref().display(), | ||
| stop_discovery_at.display() | ||
| "No Python version file found in ancestors of working directory: {}", | ||
| working_directory.as_ref().display() | ||
| ); | ||
| } | ||
| } else { | ||
| debug!( | ||
| "No Python version file found in ancestors of working directory: {}", | ||
| working_directory.as_ref().display() | ||
| ); | ||
| } | ||
| // Not found in directory or its ancestors. Looking in user-level config. | ||
| return Ok(match user_uv_config_dir() { | ||
| Some(user_dir) => Self::discover_user_config(user_dir, options) | ||
| .await? | ||
| .or(None), | ||
| None => None, | ||
| }); | ||
| local | ||
| }).flatten().or_else(|| { | ||
| // Search for a global config | ||
| Self::find_global(options) | ||
| }) else { | ||
| return Ok(None); | ||
| }; | ||
|
|
||
| if options.no_config { | ||
|
|
@@ -111,20 +124,9 @@ impl PythonVersionFile { | |
| Self::try_from_path(path).await | ||
| } | ||
|
|
||
| pub async fn discover_user_config( | ||
| user_config_working_directory: impl AsRef<Path>, | ||
| options: &DiscoveryOptions<'_>, | ||
| ) -> Result<Option<Self>, std::io::Error> { | ||
| if !options.no_config { | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We'll respect the |
||
| if let Some(path) = | ||
| Self::find_in_directory(user_config_working_directory.as_ref(), options) | ||
| .into_iter() | ||
| .find(|path| path.is_file()) | ||
| { | ||
| return Self::try_from_path(path).await; | ||
| } | ||
| } | ||
| Ok(None) | ||
| fn find_global(options: &DiscoveryOptions<'_>) -> Option<PathBuf> { | ||
| let user_config_dir = user_uv_config_dir()?; | ||
|
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure why this was passed in previously. |
||
| Self::find_in_directory(&user_config_dir, options) | ||
| } | ||
|
|
||
| fn find_nearest(path: impl AsRef<Path>, options: &DiscoveryOptions<'_>) -> Option<PathBuf> { | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -57,16 +57,13 @@ pub(crate) async fn pin( | |||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| let version_file = if global { | ||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It seems like the previous behavior was that a global pin would only be updated if
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, good point. Confusingly,
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is because we only read from this file. For construction of a new file, we use this uv/crates/uv/src/commands/python/pin.rs Lines 188 to 200 in 6c5b1e8
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The previous code was just superfluous
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah never mind, I was misunderstanding the change to |
||||||||||||||||||||||||||||
| if let Some(path) = user_uv_config_dir() { | ||||||||||||||||||||||||||||
| PythonVersionFile::discover_user_config(path, &VersionFileDiscoveryOptions::default()) | ||||||||||||||||||||||||||||
| .await | ||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||
| Ok(None) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| } else { | ||||||||||||||||||||||||||||
| PythonVersionFile::discover(project_dir, &VersionFileDiscoveryOptions::default()).await | ||||||||||||||||||||||||||||
| }; | ||||||||||||||||||||||||||||
| // Search for an existing file, we won't necessarily write to this, we'll construct a target | ||||||||||||||||||||||||||||
| // path if there's a request later on. | ||||||||||||||||||||||||||||
| let version_file = PythonVersionFile::discover( | ||||||||||||||||||||||||||||
| project_dir, | ||||||||||||||||||||||||||||
| &VersionFileDiscoveryOptions::default().with_no_local(global), | ||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||
| .await; | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| if rm { | ||||||||||||||||||||||||||||
| let Some(file) = version_file? else { | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These messages are just indented, we don't want to show them when
no_localis enabled.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you mean by just indented?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code is literally indented, there are no changes otherwise.