Update --python to accept paths to executables in environments
#17954
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
zanieb
commented
May 8, 2025
| // to deny cases like `.venv/bin/foo` | ||
| target | ||
| .file_name() | ||
| .is_some_and(|name| name.starts_with("python")) |
Member
Author
There was a problem hiding this comment.
We could be more strict here, and require python.exe on Windows and python | python3 | python3.XX on Unix. The last case is a bit annoying. We use a regular expression in uv, but I think that's probably not worth moving here yet? We might end up needing it here for nice error messages for other cases regardless though.
Contributor
|
Member
Author
|
I'm going to mark this as ready for review, for consensus on an approach. I'll add test coverage and update documentation before merging. |
zanieb
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
Member
|
(I haven't reviewed yet — currently in an airport on my phone — but I'm on board with the idea!) |
MichaReiser
approved these changes
May 12, 2025
Member
MichaReiser
left a comment
MichaReiser
left a comment
There was a problem hiding this comment.
This looks good to me. We should update the CLI and settings documentation
| // and search for a virtual environment in the root directory. Ideally, we'd | ||
| // invoke the target to determine `sys.prefix` here, but that's more complicated | ||
| // and may be deferred to uv. | ||
| .is_file(target) |
Member
There was a problem hiding this comment.
Should we also test if the file has the executable bit set (path_metadata instead of is_file)
Member
Author
There was a problem hiding this comment.
Seems reasonable, but is a little more painful cross-platform for little gain since we're just going to fail downstream anyway if we can't find site-packages
Member
Author
There was a problem hiding this comment.
Let me know if you think it's worth pursuing regardless.
zanieb
force-pushed
the
zb/resolve-python
branch
3 times, most recently
from
c2f8c21 to
6235433
Compare
AlexWaygood
approved these changes
May 12, 2025
zanieb
changed the title
--python to accept paths to executables in virtual environments--python to accept paths to executables in environments
dcreager
added a commit
that referenced
this pull request
May 13, 2025
…eepish * origin/main: [ty] Induct into instances and subclasses when finding and applying generics (#18052) [ty] Allow classes to inherit from `type[Any]` or `type[Unknown]` (#18060) [ty] Allow a class to inherit from an intersection if the intersection contains a dynamic type and the intersection is not disjoint from `type` (#18055) [ty] Narrowing for `hasattr()` (#18053) Update reference documentation for `--python-version` (#18056) [`flake8-bugbear`] Ignore `B028` if `skip_file_prefixes` is present (#18047) [`airflow`] Apply try-catch guard to all AIR3 rules (`AIR3`) (#17887) [`pylint`] add fix safety section (`PLW3301`) (#17878) Update `--python` to accept paths to executables in virtual environments (#17954) [`pylint`] add fix safety section (`PLE4703`) (#17824) [`ruff`] Implement a recursive check for `RUF060` (#17976) [`flake8-use-pathlib`] `PTH*` suppress diagnostic for all `os.*` functions that have the `dir_fd` parameter (#17968) [`refurb`] Mark autofix as safe only for number literals in `FURB116` (#17692) [`flake8-simplify`] Fix `SIM905` autofix for `rsplit` creating a reversed list literal (#18045) Avoid initializing progress bars early (#18049)
dcreager
added a commit
that referenced
this pull request
May 13, 2025
…eep-dish * origin/main: [ty] Infer parameter specializations of generic aliases (#18021) [ty] Understand homogeneous tuple annotations (#17998) [ty] Induct into instances and subclasses when finding and applying generics (#18052) [ty] Allow classes to inherit from `type[Any]` or `type[Unknown]` (#18060) [ty] Allow a class to inherit from an intersection if the intersection contains a dynamic type and the intersection is not disjoint from `type` (#18055) [ty] Narrowing for `hasattr()` (#18053) Update reference documentation for `--python-version` (#18056) [`flake8-bugbear`] Ignore `B028` if `skip_file_prefixes` is present (#18047) [`airflow`] Apply try-catch guard to all AIR3 rules (`AIR3`) (#17887) [`pylint`] add fix safety section (`PLW3301`) (#17878) Update `--python` to accept paths to executables in virtual environments (#17954) [`pylint`] add fix safety section (`PLE4703`) (#17824) [`ruff`] Implement a recursive check for `RUF060` (#17976) [`flake8-use-pathlib`] `PTH*` suppress diagnostic for all `os.*` functions that have the `dir_fd` parameter (#17968) [`refurb`] Mark autofix as safe only for number literals in `FURB116` (#17692) [`flake8-simplify`] Fix `SIM905` autofix for `rsplit` creating a reversed list literal (#18045) Avoid initializing progress bars early (#18049)
Glyphack
pushed a commit
to Glyphack/ruff
that referenced
this pull request
May 21, 2025
…nts (astral-sh#17954) ## Summary Updates the `--python` flag to accept Python executables in virtual environments. Notably, we do not query the executable and it _must_ be in a canonical location in a virtual environment. This is pretty naive, but solves for the trivial case of `ty check --python .venv/bin/python3` which will be a common mistake (and `ty check --python $(which python)`) I explored this while trying to understand Python discovery in ty in service of astral-sh/ty#272, I'm not attached to it, but figure it's worth sharing. As an alternative, we can add more variants to the `SearchPathValidationError` and just improve the _error_ message, i.e., by hinting that this looks like a virtual environment and suggesting the concrete alternative path they should provide. We'll probably want to do that for some other cases anyway (e.g., `3.13` as described in the linked issue) This functionality is also briefly mentioned in astral-sh/ty#193 Closes astral-sh/ty#318 ## Test Plan e.g., ``` uv run ty check --python .venv/bin/python3 ``` needs test coverage still
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Updates the
--pythonflag to accept Python executables in virtual environments. Notably, we do not query the executable and it must be in a canonical location in a virtual environment. This is pretty naive, but solves for the trivial case ofty check --python .venv/bin/python3which will be a common mistake (andty check --python $(which python))I explored this while trying to understand Python discovery in ty in service of astral-sh/ty#272, I'm not attached to it, but figure it's worth sharing.
As an alternative, we can add more variants to the
SearchPathValidationErrorand just improve the error message, i.e., by hinting that this looks like a virtual environment and suggesting the concrete alternative path they should provide. We'll probably want to do that for some other cases anyway (e.g.,3.13as described in the linked issue)This functionality is also briefly mentioned in astral-sh/ty#193
Closes astral-sh/ty#318
Test Plan
e.g.,
needs test coverage still