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

[POC] ruff server Expand format action capability #11803

Closed
wants to merge 1 commit into from

Conversation

T-256
Copy link
Contributor

@T-256 T-256 commented Jun 8, 2024

Summary

implement #11756 (comment) which is currently not ready for notebook documents:

{
    "ruff.format.action": {
        "fix": true,
        "isort": true,
        "style": true,
    }
}

This PR also moves ruff.fixAll and ruff.organizeImports under ruff.sourceAction superset.

Test Plan

Not tested yet

(self-note: also mention revealment issues on ruff-vscode/ruff-lsp)

@@ -165,3 +165,44 @@ pub(crate) fn fix_all(
.collect())
}
}

pub(crate) fn parse_all(query: &DocumentQuery, fixes: Fixes) -> DocumentQuery {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

feel free to rewrite this function. I think it could be better implementation at here.

@T-256 T-256 force-pushed the format-actions branch 3 times, most recently from 56e3832 to 499941b Compare June 8, 2024 16:53
@T-256 T-256 force-pushed the format-actions branch from 499941b to 357037a Compare June 8, 2024 17:03
Copy link
Contributor

github-actions bot commented Jun 8, 2024

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

@snowsignal snowsignal added the server Related to the LSP server label Jun 10, 2024
@snowsignal
Copy link
Contributor

Thank you for opening this! I'll give this a through review tomorrow.

@MichaReiser MichaReiser requested a review from snowsignal June 11, 2024 06:20
@charliermarsh
Copy link
Member

Meta note: we should decide whether we want to support this functionality before reviewing the implementation itself.

@T-256
Copy link
Contributor Author

T-256 commented Jun 11, 2024

Meta note: we should decide whether we want to support this functionality before reviewing the implementation itself.

For better decision, here is list of issues that this change may have affect on them:

Also, in my preference, I don't have any code action/format on save enablement, instead After bunch of time and complete write on that file I manually run fixAll, orginizeImport and Format Document commands to finalizing codes. I'll be happy to have these three commands in one place and by only executing Format Document all others executed.

@T-256 T-256 changed the title ruff server Expand format action capability [POC] ruff server Expand format action capability Jun 11, 2024
@gegoune
Copy link

gegoune commented Jun 11, 2024

As a user I would like to have all of those three actions executed on save. At the moment I am relying on external tool to provide some of that automation (for Neovim) and would really like to have it streamlined.

I can imagine race conditions and ordering issues resulting from using two tools interacting.

Basically, to phrase it differently, since I am already running ruff server I would like it to handle --fix as well (instead of having to also run it separately). Not sure if that's against LSP protocol though.

@snowsignal
Copy link
Contributor

@T-256 We're still deliberating on this, but we should have an update soon.

@MichaReiser
Copy link
Member

I'm commenting here because it seems that most of the discussion happened on the PR to this point and not in the issue.

I agree that this solves a real user problem, but I'm not sure if this, or even a custom command that groups these different actions is the right solution.

I've two main concerns:

First, the new settings don't work with range formatting. Ruff doesn't support applying organize imports or fixing stylistic lints to a sub range. These operations always apply to the entire document. That means that we either can't support range formatting anymore if the options are enabled or that ruff would always "format" the entire document, ignoring the users format changed lines only setting. I think either isn't a good outcome.

The alternative of having a custom command that runs formatting, isort, and stylistic fixes at once suffers from the same problem where.

Second, there are now multiple ways to configure the same behavior, except that the behavior is slightly different. My worry is that users will copy paste together their configuration without fully understanding what they're configuring. I do that too. But I worry that this will lead to "almost" functional configurations that are then hard to debug. What if they have organize imports on save enabled in their VS code settings AND included import sorting as part of formatting. Maybe everything goes well and Ruff just spends a few cycles doing unnecessary import resorting or it leads to very subtle bugs that are hard to reproduce.

That's why I prefer that we have one way to configure a specific behavior over multiple ones.

Now, this doesn't solve the problem for editors that don't support running multiple commands on save. Are there editor specific solutions that we could implement (outside of ruff_server), e.g. a custom plugin? Are there existing issues for these editors for adding support for running multiple commands on save? What's their progress?

@T-256
Copy link
Contributor Author

T-256 commented Jun 12, 2024

That means that we either can't support range formatting anymore if the options are enabled or that ruff would always "format" the entire document

As I said in #11756 (comment), Only Format Document will respect these settings, Format Selection or Format changed lines on save ignores these settings.

Your second concern is True, and I agree to it.

This was just a POC of idea I had in my mind. I close this PR because of it comes with few UX complexity.

@T-256 T-256 closed this Jun 12, 2024
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.

5 participants