-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Add search paths info to unresolved import diagnostics #20040
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
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
This solution called |
Thanks for working on this! @AlexWaygood can you take the lead on review here? |
Sure |
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.
Thanks, this looks great! I pushed a few minor tweaks to the PR.
It would be great to add a few more integration tests that have extra-paths set and/or an editable install.
SearchPathInner::Extra(_) => { | ||
"extra search path specified on the CLI or in your config file" | ||
} | ||
SearchPathInner::FirstParty(_) => "first-party code", | ||
SearchPathInner::StandardLibraryCustom(_) => { | ||
"custom stdlib stubs specified on the CLI or in your config file" | ||
} |
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.
Ideally I feel like we'd be able to tell the user whether it was a CLI option or a config-file setting that caused us to look at an extra search path (or a custom stdlib search paths) during module resolution. But that would involve a fair bit of refactoring probably; I think it's best to leave it out of this PR.
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.
LGTM with one minor nit and passing CI.
// Add search paths information to the diagnostic | ||
// Use the same search paths function that is used in actual module resolution | ||
let search_paths: Vec<_> = | ||
search_paths(self.db(), ModuleResolveMode::StubsAllowed).collect(); |
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.
Why collect all the search paths into memory first? It seems a little gratuitous. You could do let search_paths = search_paths(..).peekable();
and then if search_paths.peek().is_some() {
and for (index, path) in search_paths.enumerate() {
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.
updated by peekable. It seems requires more review to trigger next CI test. But the test looks fine on my computer now.
Thank you!!! |
* main: [`ruff`] Preserve relative whitespace in multi-line expressions (`RUF033`) (astral-sh#19647) [ty] Optimize TDD atom ordering (astral-sh#20098) [`airflow`] Extend `AIR311` and `AIR312` rules (astral-sh#20082) [ty] Preserve qualifiers when accessing attributes on unions/intersections (astral-sh#20114) [ty] Fix the inferred interface of specialized generic protocols (astral-sh#19866) [ty] Infer slightly more precise types for comprehensions (astral-sh#20111) [ty] Add more tests for protocols (astral-sh#20095) [ty] don't eagerly unpack aliases in user-authored unions (astral-sh#20055) [`flake8-use-pathlib`] Update links to the table showing the correspondence between `os` and `pathlib` (astral-sh#20103) [`flake8-use-pathlib`] Make `PTH100` fix unsafe because it can change behavior (astral-sh#20100) [`flake8-use-pathlib`] Delete unused `Rule::OsSymlink` enabled check (astral-sh#20099) [ty] Add search paths info to unresolved import diagnostics (astral-sh#20040) [`flake8-logging-format`] Add auto-fix for f-string logging calls (`G004`) (astral-sh#19303) Add a `ScopeKind` for the `__class__` cell (astral-sh#20048) Fix incorrect D413 links in docstrings convention FAQ (astral-sh#20089) [ty] Refactor inlay hints structure to use separate parts (astral-sh#20052)
…h#20040) Fixes astral-sh/ty#457 --------- Co-authored-by: Alex Waygood <[email protected]>
Fixes astral-sh/ty#457