Skip to content

Drop Python version range enforcement from PythonVersion::from_str - #7264

Merged
zanieb merged 1 commit into
mainfrom
zb/python-version-enforcement
Sep 10, 2024
Merged

zanieb merged 1 commit into
mainfrom
zb/python-version-enforcement

Conversation

@zanieb

@zanieb zanieb commented Sep 10, 2024 •

Copy link
Copy Markdown
Member

This caused some problems earlier, as it prevented us from listing Python versions <3.7 which seems weird (see #7131 (comment))

I'm worried that without this the changes to installation key parsing in #7263 would otherwise be too restrictive.

I think if we want to enforce these ranges, we should do so separately from the parse step.

@zanieb

zanieb commented Sep 10, 2024

Copy link
Copy Markdown
Member Author

@konstin looking to add this validation somewhere but not sure where it belongs

It looks like this error variant is never used?

#[error("Python {python_version} is not supported. Please use Python 3.8 or newer.")]
UnsupportedPythonVersion { python_version: String },

We probably need to check this during version requests too?

For future reference, explicit validation looks like this

diff --git a/crates/uv-python/src/python_version.rs b/crates/uv-python/src/python_version.rs
index 6c4a99487..db83bb4ac 100644
--- a/crates/uv-python/src/python_version.rs
+++ b/crates/uv-python/src/python_version.rs
@@ -165,6 +165,18 @@ impl PythonVersion {
         Self::from_str(format!("{}.{}", self.major(), self.minor()).as_str())
             .expect("dropping a patch should always be valid")
     }
+
+    /// Returns an error if the Python version is not supported.
+    pub fn check_supported(&self) -> Result<(), String> {
+        if version.version < Version::new([3, 7]) {
+            return Err(format!("Python version `{s}` must be >= 3.7"));
+        }
+        if version.version >= Version::new([4, 0]) {
+            return Err(format!("Python version `{s}` must be < 4.0"));
+        }
+
+        Ok(())
+    }
 }
 
 #[cfg(test)]

@zanieb
zanieb marked this pull request as ready for review September 10, 2024 18:15
@konstin

konstin commented Sep 10, 2024

Copy link
Copy Markdown
Member

We should be filtering out too old interpreters at

if sys.version_info < (3, 7):

@zanieb

zanieb commented Sep 10, 2024

Copy link
Copy Markdown
Member Author

Ah okay so it's constructed by the deserializer. Lgtm then.

@zanieb
zanieb merged commit 533c7e3 into main Sep 10, 2024
@zanieb
zanieb deleted the zb/python-version-enforcement branch September 10, 2024 18:34
zanieb added a commit that referenced this pull request Sep 10, 2024
…7269)

Follows test cases in #7265 and validation removal in
#7264

It turns out we don't have good error messages for these as-is.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants