-
Notifications
You must be signed in to change notification settings - Fork 1.6k
[ty] Inlay hint call argument location #20349
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
[ty] Inlay hint call argument location #20349
Conversation
Diagnostic diff on typing conformance testsNo changes detected when running ty on typing conformance tests ✅ |
|
|
Getting this error in playground. It seems like i have broken the goto as well as the inlay hint location still not working. will look into this more Uncaught Error: can't access property "monaco", this is undefined
mapNavigationTarget@http://localhost:5173/src/Editor/Editor.tsx:464:13
mapNavigationTargets@http://localhost:5173/src/Editor/Editor.tsx:495:22
provideDefinition@http://localhost:5173/src/Editor/Editor.tsx:391:21 |
I'd be happy to keep the playground changes in a separate PR to get the LSP changes in. |
|
I fixed this error, was a small issue, I fixed in the "Fix Playground" commit |
Nice. Does that mean this PR is now ready for review or is there more to do? |
|
I am just looking over it for the final time now, will make it ready for review today |
|
@Gankra is this a PR you could take a look at? If not, that's fine. It might just take me a few days before I get to it because I'm still catching up with notifications after my PTO (and have some other time sensitive things that I need to do first) |
Gankra
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.
The basic approach seems reasonable! I was going to complain that this approach doesn't properly play with stub-mapping and the goto-definition vs goto-declaration distinction, but after looking at the fields provided by the spec it seems like it really does only let you pick one "target" for the inlay hint.
That said the implementation you've chosen implements goto-declaration semantics which is not the default for a cmd+click, which prefers goto-definition. I think it would be fine to have that be a followup, as this is already seemingly strictly better than what pylance does!
|
working fine in vscode, just need to look into the playground not working |
|
really have no clue why it isnt working |
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.
really have no clue why it isnt working
Do you mean that the inlay's aren't clickable. I somewhat suspect that this isn't implemented in monaco.
I tried to find an inlay example but couldn't find any (the TypeScript playground also doesn't use inlays). It's quiet possible that monaco doesn't support clickable inlays?
playground/ty/src/Editor/Editor.tsx
Outdated
| label: hint.markdown, | ||
| label: hint.label.map((part) => ({ | ||
| label: part.label, | ||
| location: |
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.
Given that this doesn't work in Monaco (it seems) I'm inclined to not set location and instead at a comment saying that. As of today (date), location isn't supported by Monaco which is why the field is ommited in the playground.
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.
yeah that makes sense, will update. Thanks
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.
I think it would be good to keep all of the updates to ty_wasm, just incase in the future monaco supports this, what do you think?
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.
Yeah, I think that's fine. I also don't expect the ty_wasm code to add too much overhead. It only maps a few locations
|
@Gankra would you mind taking another look at this PR (The playground changes look good to me) |
|
I understand this isn't important for the beta but just making sure this doesn't get lost. |
|
Thanks. I think it got lost 😓 I'll take another look tomorrow and another ping to @Gankra |
|
Thank you |
|
Let me know when this is ready for re-review (you can request a review) |
7441715 to
3006711
Compare
* origin/main: (26 commits) Mention `force-exclude` in "Configuration > Python file discovery" (#21500) Avoid syntax error when formatting attribute expressions with outer parentheses, parenthesized value, and trailing comment on value (#20418) [ty] suppress invalid suggestions in import statements (#21484) Limit `eglot-format` hook to eglot-managed Python buffers (#21459) Adjust own-line comment placement between branches (#21185) [ty] Subscript assignment diagnostics follow-up (#21452) [ty] Inlay hint call argument location (#20349) [ty] Use `CompactStr` for `StringLiteralType` (#21497) Update CodSpeedHQ/action action to v4.3.4 (#21488) Update salsa digest to a885bb4 (#21486) Update dependency ruff to v0.14.5 (#21489) Update astral-sh/setup-uv action to v7.1.3 (#21487) Update Rust crate get-size2 to v0.7.2 (#21490) Update Rust crate indicatif to v0.18.3 (#21491) Update Rust crate quick-junit to v0.5.2 (#21492) Update taiki-e/install-action action to v2.62.52 (#21493) Fix analyze graph tests on windows (#21481) `analyze`: Add option to skip over imports in `TYPE_CHECKING` blocks (#21472) [ty] Dataclasses: `__hash__` semantics and `unsafe_hash` (#21470) [ty] Dataclass transform: complete set of parameters (#21474) ...
* dcreager/coolable: (31 commits) mdformat don't panic Mention `force-exclude` in "Configuration > Python file discovery" (#21500) Avoid syntax error when formatting attribute expressions with outer parentheses, parenthesized value, and trailing comment on value (#20418) [ty] suppress invalid suggestions in import statements (#21484) known discrepancy TODO α-rename todo equiv too Limit `eglot-format` hook to eglot-managed Python buffers (#21459) Adjust own-line comment placement between branches (#21185) [ty] Subscript assignment diagnostics follow-up (#21452) [ty] Inlay hint call argument location (#20349) [ty] Use `CompactStr` for `StringLiteralType` (#21497) Update CodSpeedHQ/action action to v4.3.4 (#21488) Update salsa digest to a885bb4 (#21486) Update dependency ruff to v0.14.5 (#21489) Update astral-sh/setup-uv action to v7.1.3 (#21487) Update Rust crate get-size2 to v0.7.2 (#21490) Update Rust crate indicatif to v0.18.3 (#21491) Update Rust crate quick-junit to v0.5.2 (#21492) ...
Summary
Adds location to the inlay hint label part for call arguments. This allows "goto" on the inlay hint.
inlay-hint-call-argument-location.webm
This has not been added to the playground as Monaco doesn't support it.
Test Plan
updated a test in
ty_server. But other than that im not sure the best way to approach tests for this.