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

Improvement Idea: sort code actions by specificity #210

Closed
MrGreenTea opened this issue Aug 8, 2023 · 7 comments
Closed

Improvement Idea: sort code actions by specificity #210

MrGreenTea opened this issue Aug 8, 2023 · 7 comments
Labels
enhancement New feature or request

Comments

@MrGreenTea
Copy link
Contributor

MrGreenTea commented Aug 8, 2023

Hi, thanks for this great integration. It works really well.

When I have an unused import and want to auto remove it I get a list similar to this:

image

In my opinion I would prefer the cursor/line specific code action to be at position 1 as it applies directly to the current cursor position.
I believe the order should be

  1. Remove unused import (currently at 3)
  2. Disable for this line (currently at 4)
  3. Organize Imports (currently at 1)
  4. Fix All ((currently at 2)

I've taken a look at the server implementation and it seems the order comes from the return of code_action in server.py.

If first the Ruff: Autofix and "Disable for this line" are added to the list and then Organize Imports and Fix All it looks like this:

image

What is your opinion on this? Would you be interested in me providing these changes as a PR?

@MrGreenTea
Copy link
Contributor Author

MrGreenTea commented Aug 8, 2023

I was only thinking about the order of the autofix and the suggestion to disable it. I think the way it is right now makes more sense, where the autofixes are shown first and then in a separate "block" the disables. But maybe you think different?

This is what I mean and the way I prefer it:
image

This is how it could also look, sorted by diagnostic code, which I find quite confusing actually:
image

@zanieb
Copy link
Member

zanieb commented Aug 14, 2023

I like the idea! Would you be interested in opening a pull request? @charliermarsh may have an opinion as well.

@zanieb zanieb added the enhancement New feature or request label Aug 14, 2023
@dhruvmanila
Copy link
Member

This is an interesting and useful idea but a point to be noted is that the client could alter the order of code actions. The server may provide the code actions in some order but the client implementation (in this case the Neovim LSP client + Telescope fuzzy finder) might alter the order to display the code actions. In an ideal scenario, the client wouldn't update the order but it's a possibility.

@charliermarsh
Copy link
Member

This seems reasonable to me. I'm not sure how much we can do to order the single-code fixes (e.g., how should we order F401 vs. F841?), but putting the Fix All and Organize Imports at the end seems reasonable.

@MrGreenTea
Copy link
Contributor Author

I like the idea! Would you be interested in opening a pull request? @charliermarsh may have an opinion as well.

Yes I will create a PR for this in the next days. Glad you like the idea as well.

This is an interesting and useful idea but a point to be noted is that the client could alter the order of code actions. The server may provide the code actions in some order but the client implementation (in this case the Neovim LSP client + Telescope fuzzy finder) might alter the order to display the code actions. In an ideal scenario, the client wouldn't update the order but it's a possibility.

Yeah there is nothing we can do about that though, right? Perhaps we should look at this as the suggested order. It's also interesting to me how this interacts with other servers that provide code actions at that location.

MrGreenTea added a commit to MrGreenTea/ruff-lsp that referenced this issue Sep 18, 2023
This way code actions for specific diagnostic codes are suggested
before more general ones like 'Fix: All' or 'Organize Imports'.

See issue astral-sh#210 for discussions
@MrGreenTea
Copy link
Contributor Author

I've now gotten around to implementing the change (which was just moving two code blocks above the others in the code_action handler.

I'm a bit at a loss at how to write tests for this though. If someone could give me a pointer in the right direction I'd be very glad, otherwise I will need to find an hour or so some other time and dig into it myself. I've opened a WIP Pull Request: #245

charliermarsh pushed a commit that referenced this issue Sep 27, 2023
## Summary

It changes the sort order sent to clients. This way code actions for
specific diagnostic codes are suggested before more general ones like
'Fix: All' or 'Organize Imports'.

See issue #210 for discussions
@charliermarsh
Copy link
Member

Closed by #245.

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

No branches or pull requests

4 participants