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

ruff server now supports the source.organizeImports source action #10652

Merged
merged 3 commits into from
Apr 4, 2024

Conversation

snowsignal
Copy link
Contributor

Summary

This builds on top of the work in #10597 to support Ruff: Organize imports as an available source action.

To do this, we have to support Clone-ing for linter settings, since we need to modify them in place to select import-related diagnostics specifically (I001 and I002).

Test Plan

organizeImports.Test.Plan.mp4

@snowsignal snowsignal added the server Related to the LSP server label Mar 29, 2024
Copy link
Contributor

github-actions bot commented Mar 29, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

crates/ruff_server/src/server/api/requests/code_action.rs Outdated Show resolved Hide resolved
mut action: types::CodeAction,
document: &crate::edit::Document,
url: &types::Url,
mut linter_settings: ruff_linter::settings::LinterSettings,
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, it's unfortunate that this requires making LinterSettings cloneable but I haven't been able to come up with an alternative solution that not at least requires cloning individual fields. Ideally it would be possible to call check with the desired rules, but that's a larger refactor and is probably not worth it.

I don't feel strongly but I have a slight preference to pass in a LinterSetting and move the cloning into resolve_edit_for_organize_imports.

Copy link
Member

Choose a reason for hiding this comment

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

Could we do something like the following?

  1. Take a snapshot of existing settings
  2. Mutate it temporarily
  3. Restore the settings with the snapshot

I'm not sure how involved this is or if it's better than cloning. That said, I wouldn't recommend doing in this PR but something to think about if we need or want to refactor this in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MichaReiser @dhruvmanila I'll make a follow-up issue to look into how we could improve this for the future.

@MichaReiser
Copy link
Member

Would you mind testing that adding the organizeImports action (ruff or the global one) into the on save action works as expected

{
  "[python]": {
    "editor.formatOnSave": true,
    "editor.codeActionsOnSave": {
      "source.fixAll": "explicit",
      "source.organizeImports": "explicit"
    },
    "editor.defaultFormatter": "charliermarsh.ruff"
  }
}

(or using the ruff specific actions)

Base automatically changed from jane/server/code-actions/source-level to main April 3, 2024 16:22
@snowsignal snowsignal force-pushed the jane/server/code-actions/organize-imports branch from 4d50aa6 to 9145d53 Compare April 4, 2024 21:44
@snowsignal
Copy link
Contributor Author

@MichaReiser I've confirmed that the configuration you've provided works as expected. However, the ruff-specific configuration is not working, but for an unrelated reason, which I'll address in a follow-up issue.

@snowsignal snowsignal enabled auto-merge (squash) April 4, 2024 22:14
@snowsignal snowsignal merged commit d050d6d into main Apr 4, 2024
17 checks passed
@snowsignal snowsignal deleted the jane/server/code-actions/organize-imports branch April 4, 2024 22:20
snowsignal added a commit that referenced this pull request Apr 5, 2024
…ts, and formatting (#10654)

## Summary

This builds off of the work in
#10652 to implement a command
executor, backwards compatible with the commands from the previous LSP
(`ruff.applyAutofix`, `ruff.applyFormat` and
`ruff.applyOrganizeImports`).

This involved a lot of refactoring and tweaks to the code action
resolution code - the most notable change is that workspace edits are
specified in a slightly different way, using the more general `changes`
field instead of the `document_changes` field (which isn't supported on
all LSP clients). Additionally, the API for synchronous request handlers
has been updated to include access to the `Requester`, which we use to
send a `workspace/applyEdit` request to the client.

## Test Plan



https://github.com/astral-sh/ruff/assets/19577865/7932e30f-d944-4e35-b828-1d81aa56c087
Glyphack pushed a commit to Glyphack/ruff that referenced this pull request Apr 12, 2024
…astral-sh#10652)

## Summary

This builds on top of the work in
astral-sh#10597 to support `Ruff: Organize
imports` as an available source action.

To do this, we have to support `Clone`-ing for linter settings, since we
need to modify them in place to select import-related diagnostics
specifically (`I001` and `I002`).

## Test Plan


https://github.com/astral-sh/ruff/assets/19577865/04282d01-dfda-4ac5-aa8f-6a92d5f85bfd
Glyphack pushed a commit to Glyphack/ruff that referenced this pull request Apr 12, 2024
…ts, and formatting (astral-sh#10654)

## Summary

This builds off of the work in
astral-sh#10652 to implement a command
executor, backwards compatible with the commands from the previous LSP
(`ruff.applyAutofix`, `ruff.applyFormat` and
`ruff.applyOrganizeImports`).

This involved a lot of refactoring and tweaks to the code action
resolution code - the most notable change is that workspace edits are
specified in a slightly different way, using the more general `changes`
field instead of the `document_changes` field (which isn't supported on
all LSP clients). Additionally, the API for synchronous request handlers
has been updated to include access to the `Requester`, which we use to
send a `workspace/applyEdit` request to the client.

## Test Plan



https://github.com/astral-sh/ruff/assets/19577865/7932e30f-d944-4e35-b828-1d81aa56c087
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Related to the LSP server
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants