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

Check Workspace Edit ResourceOps #15101

Merged
merged 4 commits into from Jun 28, 2023
Merged

Check Workspace Edit ResourceOps #15101

merged 4 commits into from Jun 28, 2023

Conversation

alibektas
Copy link
Member

@alibektas alibektas commented Jun 21, 2023

PR fixes #14780

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 21, 2023
Fixes rust-lang#14780 . This commit introduces guards for checking if the client supports ResourceOperations for operations to use them.
@Veykril
Copy link
Member

Veykril commented Jun 21, 2023

Regarding CI failure, the slow tests are failing because the capabilities are not set by default. You'll have to add them here https://github.com/Veykril/rust-analyzer/blob/00b9d9faf43936d0c4393870ffd14580078924d0/crates/rust-analyzer/tests/slow-tests/support.rs#L109-L144

@Veykril
Copy link
Member

Veykril commented Jun 28, 2023

@bors r+

@bors
Copy link
Collaborator

bors commented Jun 28, 2023

📌 Commit f8518a6 has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Collaborator

bors commented Jun 28, 2023

⌛ Testing commit f8518a6 with merge 891331c...

@bors
Copy link
Collaborator

bors commented Jun 28, 2023

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

@bors bors merged commit 891331c into rust-lang:master Jun 28, 2023
8 checks passed
@alibektas alibektas mentioned this pull request Jun 30, 2023
bors added a commit that referenced this pull request Jun 30, 2023
Purge of unwraps

Removes unnecessary unwraps that I have overlooked in #15101 ( fixes #15176 )
@nemethf
Copy link
Contributor

nemethf commented Jul 15, 2023

I'm just thinking aloud, but I wonder if it would be better not to offer a code-action to the client that would later lead to the error of "Client does not support {} capability". What do you think, @alibektas?

@alibektas
Copy link
Member Author

This was how I first thought I would do it as finding which code actions can be shown to the client requires unnecessary computation. There were two aspects that spoke against it ( which I discussed with more experienced contributors and they actually dissuaded me to not do it the way you also propose ) let me list them :

  1. The case where a client doesn't support the said ResourceOperations is not so common, thus making any clever approach possibly an overkill.
  2. Future-proofness of the current version : Had we decided for the early catching of unusable code actions, we would have to make sure that we obey client's Resource Operation capabilities this is I believe more error prone.

So all in all it is doable but less favorable

@nemethf
Copy link
Contributor

nemethf commented Jul 15, 2023

Thanks for the explanation. It all makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

r-a does not check the resourceOptions value reported by the client
5 participants