-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Infer the Python version from --python=<system installation> on Unix
#18550
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
Conversation
7a440de to
14ea91b
Compare
|
|
MichaReiser
left a comment
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 makes it hard if the user is on a windows machine? I'd prefer if the CLI has consistent behavior across platforms.
Apologies — I left several comments about this in the source code but forgot to mention it in my PR description. On Unix, the
Yeah, I also wasn't totally sure if this was worth the inconsistency. I think being able to infer the Python version correctly from the minimum number of config options is very high-value, though, and chatting with @zanieb at PyCon they also thought that this was probably worth it |
|
Yeah on Windows you'd need to query the interpreter to infer the version. Or.. maybe you could get away with reading the registry and matching paths (but that seems more complicated than it's worth here!) |
yeah I definitely don't want to do that 😆 |
MichaReiser
left a comment
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.
Ty
Mainly a few comments around terminology.
| let (major, minor) = s.split_once('.').ok_or(())?; | ||
| Self::try_from((major, minor)).map_err(|_| ()) |
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.
Nit: Not sure if it's worth it because it's so simple but this is somewhat duplicated now with the serde deserialization logic below.
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.
This was bothering me too, especially because we also had duplicated logic in this third place:
ruff/crates/ty_python_semantic/src/module_resolver/typeshed.rs
Lines 331 to 346 in 3aae1cd
| fn python_version_from_versions_file_string( | |
| s: &str, | |
| ) -> Result<PythonVersion, TypeshedVersionsParseErrorKind> { | |
| let mut parts = s.split('.').map(str::trim); | |
| let (Some(major), Some(minor), None) = (parts.next(), parts.next(), parts.next()) else { | |
| return Err(TypeshedVersionsParseErrorKind::UnexpectedNumberOfPeriods( | |
| s.to_string(), | |
| )); | |
| }; | |
| PythonVersion::try_from((major, minor)).map_err(|int_parse_error| { | |
| TypeshedVersionsParseErrorKind::IntegerParsingFailure { | |
| version: s.to_string(), | |
| err: int_parse_error, | |
| } | |
| }) | |
| } |
I've unified the three implementations.
|
Apologies for the terrible image quality of this screenshot (taken on an ancient Windows machine I have hanging around that does not have -- and will never have -- Rust installed on it). But it does look like it is possible to query the metadata of a Python binary on Windows without actually invoking that binary in a subprocess. The "File version" and "Product version" fields here both contain the Python version in them (obtained here by right-clicking on the The metadata appears consistent regardless of whether the binary was installed using "official" python.org installers or using uv. From what I can tell, though, querying these |
|
Oh that's cool. |
|
That is cool indeed. cc @konstin we could use this to filter Python binaries more aggressively on Windows without querying the interpreter! |
14ea91b to
a572b1e
Compare
|
Is there anything specific you want me to re-review? |
Are you okay with my response in #18550 (comment)?
|
|
Sorry, I was in meetings. Do you still want me to re-review or are you okay, considering that you merged the change 😆 |
|
Ohh sorry! Still might be good for you to give the changes in |
|
Okay, I'll do so tomorrow |
* main: [ty] Add some "inside string" tests for `object.<CURSOR>` completions [ty] Pull types on synthesized Python files created by mdtest (#18539) Update Rust crate anstyle to v1.0.11 (#18583) [`pyupgrade`] Fix `super(__class__, self)` detection in UP008 (super-call-with-parameters) (#18478) [ty] Generate the top and bottom materialization of a type (#18594) `SourceOrderVisitor` should visit the `Identifier` part of the `PatternKeyword` node (#18635) Update salsa (#18636) [ty] Update mypy_primer doc (#18638) [ty] Improve support for `object.<CURSOR>` completions [ty] Add `CoveringNode::find_last` [ty] Refactor covering node representation [ty] Infer the Python version from `--python=<system installation>` on Unix (#18550) [`flake8-return`] Fix `RET504` autofix generating a syntax error (#18428) Fix incorrect salsa `return_ref` attribute (#18605) Move corpus tests to `ty_python_semantic` (#18609) [`pyupgrade`] Don't offer fix for `Optional[None]` in non-pep604-annotation-optional (`UP045)` or non-pep604-annotation-union (`UP007`) (#18545) [`pep8-naming`] Suppress fix for `N804` and `N805` if the recommend name is already used (#18472) [`ruff`] skip fix for `RUF059` if dummy name is already bound (unused-unpacked-variable) (#18509)


Summary
If a user passes
--python=<virtual_environment>and doesn't specify a Python version on the CLI or in a configuration file, we currently use the metadata in that virtual environment'spyvenv.cfgfile to infer the Python version we should use for resolving modules and types. We currently do not attempt to infer the Python version if a user passes--python=<system_installation>, but it's easy enough to do that as well, providing the user is not on a Windows machine. This PR implements that.Test Plan
CLI integration tests, and manual testing.
Screenshot of what this looks like on the CLI: