[ty] Respect notebook cell boundaries when adding an auto import #21322
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
MichaReiser
requested review from
AlexWaygood,
carljm,
dcreager and
sharkdp
as code owners
MichaReiser
added
server
Contributor
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
Contributor
|
MichaReiser
force-pushed
the
micha/notebook-auto-import
branch
from
7e31390 to
d5de564
Compare
MichaReiser
requested review from
BurntSushi
and removed request for
AlexWaygood,
carljm,
dcreager and
sharkdp
MichaReiser
force-pushed
the
micha/notebook-auto-import
branch
3 times, most recently
from
a9dae9e to
fbbed61
Compare
Contributor
|
Gankra
reviewed
Nov 8, 2025
Comment on lines
+10
to
+13
| [[profile.default.overrides]] | ||
| filter = 'binary(e2e)' | ||
| test-group = 'serial' | ||
|
|
Contributor
There was a problem hiding this comment.
Was this intended to be checked in?
Member
Author
There was a problem hiding this comment.
Sort of. My hope was that it makes the lsp e2e tests less flaky
Gankra
approved these changes
Nov 8, 2025
Contributor
Gankra
left a comment
Gankra
left a comment
There was a problem hiding this comment.
Interesting, when we were talking about it before I had convinced myself this would fall out more naturally from treating the cells as separate files, but I guess we're (reasonably) treating the notebooks as one file still.
MichaReiser
force-pushed
the
micha/lsp-notebooks-phase2
branch
from
249643e to
7a1fad3
Compare
MichaReiser
force-pushed
the
micha/notebook-auto-import
branch
from
fbbed61 to
b3e4db8
Compare
dhruvmanila
reviewed
Nov 12, 2025
crates/ty_ide/src/importer.rs
Outdated
Comment on lines
153
to
156
| // What we want here: Given an offset, return the range of that cell. I guess that's what containing range is | ||
| let range = source_text(self.db, self.file) | ||
| .as_notebook() | ||
| .and_then(|notebook| notebook.cell_offsets().containing_range(members.at)); |
Member
There was a problem hiding this comment.
Yes, this comment is correct. The containing_range will return the cell range that contains this offset.
dhruvmanila
reviewed
Nov 12, 2025
MichaReiser
force-pushed
the
micha/lsp-notebooks-phase2
branch
from
0f3fc0f to
86c98f8
Compare
Enable auto-import functionality for notebook cells with proper scoping to ensure import edits are restricted to the current cell rather than spanning across cells. Changes: - Update Insertion::start_of_file to accept optional within_range parameter - When provided, restricts import placement to statements within that range - Allows per-cell import handling in notebooks - Update importer logic to handle notebook cell boundaries - Find cell range for the position where import is needed - Pass cell range to restrict import insertion to current cell - Skip imports from other cells when searching for existing imports - Ensure import insertions respect cell document URIs - Add tests verifying auto-import works correctly in notebooks - Verify imports aren't added to different cells This ensures that when auto-completing in a notebook cell, suggested imports are added to the current cell only, not to import statements in other cells.
MichaReiser
force-pushed
the
micha/notebook-auto-import
branch
from
ac99544 to
d550ea9
Compare
MichaReiser
added
internal
Member
Author
|
I marked this as internal because notebook support hasn't shipped yet. So this isn't a user visible change |
dcreager
added a commit
that referenced
this pull request
Nov 14, 2025
* origin/main: (59 commits) [ty] Improve diagnostic range for `non-subscriptable` diagnostics (#21461) [ty] Improve literal promotion heuristics (#21439) [ty] Further improve details around which expressions should be deferred in stub files (#21456) [ty] Improve generic class constructor inference (#21442) [ty] Propagate type context through conditional expressions (#21443) [ty] Suppress completions when introducing names with `as` [ty] Add panic-by-default await methods to `TestServer` (#21451) [ty] name is parameter and global is a syntax error (#21312) [ty] Fixup a few details around version-specific dataclass features (#21453) [ty] Support attribute-expression `TYPE_CHECKING` conditionals (#21449) [ty] Support stringified annotations in value-position `Annotated` instances (#21447) [ty] Type inference for genererator expressions (#21437) [ty] Make `__getattr__` available for `ModuleType` instances (#21450) [ty] Increase default receive timeout in tests to 10s (#21448) [ty] Add synthetic members to completions on dataclasses (#21446) [ty] Support legacy `typing` special forms in implicit type aliases (#21433) Bump 0.14.5 (#21435) [ty] Support `type[…]` and `Type[…]` in implicit type aliases (#21421) [ty] Respect notebook cell boundaries when adding an auto import (#21322) Update PyCharm setup instructions (#21409) ...
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
The text edits for auto-completion are constrained to within the same document (because there's only a range but no URI field). Given that the LSP represents each cell as a separate document, this means that auto import edits must be limited to the same cell (we can't change an import from another cell or add an import to the first cell).
This PR does just that.
Test plan
Added e2e tests