Skip to content

Conversation

@MichaReiser
Copy link
Member

Summary

This PR centralizes the auto discovery logic for a Python environment into SearchPaths::from_settings.

This should make astral-sh/ty#611 an easy change (manly prioritizing .venv over CONDA_PREFIX).

Test Plan

Existing tests

@MichaReiser MichaReiser added internal An internal refactor or improvement ty Multi-file analysis & type inference labels Jun 25, 2025

This comment was marked as outdated.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 25, 2025

mypy_primer results

No ecosystem changes detected ✅

@codspeed-hq
Copy link

codspeed-hq bot commented Jun 25, 2025

CodSpeed WallTime Performance Report

Merging #18938 will not alter performance

Comparing micha/python-discovery-search-paths (77242e7) with main (d04e63a)

Summary

✅ 8 untouched benchmarks

@MichaReiser MichaReiser force-pushed the micha/python-discovery-search-paths branch from 763f6ac to f66c43c Compare June 25, 2025 16:25
@MichaReiser MichaReiser marked this pull request as ready for review June 25, 2025 16:33
@github-actions
Copy link
Contributor

github-actions bot commented Jun 25, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@MichaReiser
Copy link
Member Author

MichaReiser commented Jun 25, 2025

Hmm, not sure what's up with the benchmark. The auto discovery isn't run for benchmarks (it's a fixed environment) and the program settings are resolved outside the benchmark

I verified that both main and this PR perform the same search path discovery in the benchmark...

Edit: It seems the benchmark triggers on other PRs too. So maybe just a flake?

@MichaReiser MichaReiser requested a review from AlexWaygood June 26, 2025 06:26
@sharkdp sharkdp removed their request for review June 26, 2025 07:05
Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thanks! This looks basically good. I still feel like there's some confusing distinctions being introduced that I'm not totally sold on.

It would also be ideal if we could add a regression test for #18938 (comment), but it was obviously a pre-existing issue that we had no coverage there :-)

.python()
.map(|sys_prefix| {
PythonPath::IntoSysPrefix(
PythonPath::sys_prefix(
Copy link
Member

Choose a reason for hiding this comment

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

is the sys_prefix method useful if this is all it does? 😆

Copy link
Member Author

Choose a reason for hiding this comment

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

It wasn't introduced by me!

Copy link
Member

Choose a reason for hiding this comment

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

I know! 😆

Copy link
Member

@AlexWaygood AlexWaygood left a comment

Choose a reason for hiding this comment

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

Thank you! The new names make it much clearer

Co-authored-by: Alex Waygood <Alex.Waygood@Gmail.com>
@MichaReiser
Copy link
Member Author

Thank you! The new names make it much clearer

It's a shame that I'll delete the entire enum in my next PR :D

@MichaReiser MichaReiser merged commit 7638729 into main Jun 26, 2025
35 checks passed
@MichaReiser MichaReiser deleted the micha/python-discovery-search-paths branch June 26, 2025 14:39
dcreager added a commit that referenced this pull request Jun 27, 2025
* main:
  [ty] Add builtins to completions derived from scope (#18982)
  [ty] Don't add incorrect subdiagnostic for unresolved reference (#18487)
  [ty] Simplify `KnownClass::check_call()` and `KnownFunction::check_call()` (#18981)
  [ty] Add micro-benchmark for #711 (#18979)
  [`flake8-annotations`] Make `ANN401` example error out-of-the-box (#18974)
  [`flake8-async`] Make `ASYNC110` example error out-of-the-box (#18975)
  [pandas]: Fix issue on `non pandas` dataframe `in-place` usage (PD002) (#18963)
  [`pylint`] Fix `PLC0415` example (#18970)
  [ty] Add environment variable to dump Salsa memory usage stats (#18928)
  [`pylint`] Fix `PLW0108` autofix introducing a syntax error when the lambda's body contains an assignment expression (#18678)
  Bump 0.12.1 (#18969)
  [`FastAPI`] Add fix safety section to `FAST002` (#18940)
  [ty] Add regression test for leading tab mis-alignment in diagnostic rendering (#18965)
  [ty] Resolve python environment in `Options::to_program_settings` (#18960)
  [`ruff`] Fix false positives and negatives in `RUF010` (#18690)
  [ty] Fix rendering of long lines that are indented with tabs
  [ty] Add regression test for diagnostic rendering panic
  [ty] Move venv and conda env discovery to `SearchPath::from_settings` (#18938)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

internal An internal refactor or improvement ty Multi-file analysis & type inference

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants