Skip to content
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

internal: Stop eagerly resolving inlay hint text edits for VSCode #16473

Merged
merged 2 commits into from
Mar 11, 2024

Conversation

SomeoneToIgnore
Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore commented Feb 1, 2024

Send less json over the wire.
After microsoft/vscode#193124 was fixed, this change is not needed anymore.

VSCode 1.86.0 now supports double click for unresolved hint data too.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 1, 2024
@lnicola
Copy link
Member

lnicola commented Feb 1, 2024

You might need to rebase to fix the CI.

@davidbarsky what VS Code are you on?

@davidbarsky
Copy link
Contributor

You might need to rebase to fix the CI.

@davidbarsky what VS Code are you on?

1.78 right now, but 1.82 in a week or so. We can carry this as a patch, need be.

@Veykril
Copy link
Member

Veykril commented Feb 4, 2024

Fwiw there is version field in the InitializeParams that a client can populate. Haven't checked but I'd expect that VSCode populates that. We could read that out and make this conditional on that for a few months. Otherwise if we merge this we should bump the VSCode requirement of the client extension so that we force people to upgrade their VSCode to contain the fix.

@SomeoneToIgnore
Copy link
Contributor Author

I'm not in a rush and can hold this PR, as the whole point of it was to get rid of any code-specific checks.

But if you want to have it nonetheless, let me know and I will push more checks on top.

@Veykril Veykril added S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 9, 2024
After microsoft/vscode#193124 was fixed,
this change is not needed anymore.
@Veykril Veykril force-pushed the kb/remove-resolve-stub branch 2 times, most recently from f5df64b to 913e8e2 Compare March 11, 2024 09:27
@Veykril
Copy link
Member

Veykril commented Mar 11, 2024

Rebased (because of clippy issues that were fixed on master) and made the check conditional on the vscode version s owe can just merge it now.

@Veykril
Copy link
Member

Veykril commented Mar 11, 2024

Thanks!
@bors r+

@bors
Copy link
Collaborator

bors commented Mar 11, 2024

📌 Commit 0dbaccd has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Mar 11, 2024

⌛ Testing commit 0dbaccd with merge a0dd822...

@bors
Copy link
Collaborator

bors commented Mar 11, 2024

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing a0dd822 to master...

@bors bors merged commit a0dd822 into rust-lang:master Mar 11, 2024
11 checks passed
@Wilfred
Copy link
Contributor

Wilfred commented Mar 12, 2024

Looking at the upstream VS Code commit microsoft/vscode@8adf59f, I think the fix shipped in VS Code 1.86 FWIW.

@SomeoneToIgnore SomeoneToIgnore deleted the kb/remove-resolve-stub branch March 12, 2024 19:10
@Veykril
Copy link
Member

Veykril commented Mar 13, 2024

1.86 is what the code in the PR checks for

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-blocked Status: marked as blocked ❌ on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants