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

Prevent code actions on read only sources. #575

Merged
merged 1 commit into from
Mar 30, 2023

Conversation

andrewL-avlq
Copy link
Contributor

Code actions cannot be performed on read only sources, so there is no need to ask the language server for them.

Copy link
Contributor

@mickaelistria mickaelistria left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LSP spec mentions that a codeAction can return a WorkspaceEdit or a command, and those can affect other files or just affect the environment/context/configuration... We should still query the LS for those.
However, what would be good from a UX POV would be to filter out codeActions that return a WorkspaceEdit that affects read-only files.

Code actions cannot be performed on read only sources, so filter them
out of proposals.
@mickaelistria
Copy link
Contributor

I don't think the test failure is related, and the change is good. Let's merge.
Thank you!

@mickaelistria mickaelistria merged commit e49962d into eclipse:master Mar 30, 2023
@rubenporras
Copy link
Contributor

@andrewL-avlq, I have seen similar errors happening on another PRs after this PR has been merged. Could you take a look to be sure that the test failures where unrelated?

@mickaelistria
Copy link
Contributor

I'm pretty sure they're unrelated to this particular change and am currently looking at how to avoid them in tests. I suspect just reorganizing the close/delete blocks will do the work.

@mickaelistria
Copy link
Contributor

However, it doesn't look like this test cover the addition of filtering changes that affect read-only content

@andrewL-avlq
Copy link
Contributor Author

The failing tests seem to be the result of errors logged while performing AllCleanRule.finished - given that it is the NoErrorLoggedRule test rule that is setting the test as failed, a simple way to ignore such exceptions would be to order the rules (add (order=x) to the annotations) so that the NoErrorLoggedRule is 'inner'.

The test added in this PR tests the filtering of marker resolutions on markers, and fails if you set the filter to always return true.

I didn't see any other tests for the behavior of code actions - do you think more should be added?

@mickaelistria
Copy link
Contributor

The failing tests seem to be the result of errors logged

I attempted to fix it in #585 ; let's hope it works reliably.

do you think more should be added?

If you don't feel the need for more at the moment, then it's fine to stick to the current state. If need be, they will be improved later.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants