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

Run ruff format when lsp formatting is invoked #57

Merged
merged 3 commits into from
Nov 25, 2023

Conversation

mmcshane
Copy link
Contributor

@mmcshane mmcshane commented Nov 7, 2023

Adds the Subcommand enum to indicate which ruff subcommand should be executed by run_ruff. At this time, only check and format are supported. As different subcommands support different parameters, argument generation is delegated based on the specific subcommand value.

The ruff format subcommand does not currently organize imports and there does not appear to be a way to convince it to do so. Until a unified command exists the approach taken here is to format and then make a second run of ruff check that only performs import formatting.

Relates to #53.

@mmcshane mmcshane force-pushed the mpm/ruff-format branch 2 times, most recently from 9207716 to 735b2d0 Compare November 8, 2023 13:46
@JaRoSchm
Copy link

JaRoSchm commented Nov 9, 2023

If ruff decided that ruff format should not organize imports, then why should this plugin do that? I think it would be preferable do mirror exactly what ruff and ruff-lsp are doing. If someone wants to do both at once, this should be an additional setting and not the default in my opinion.

@mmcshane
Copy link
Contributor Author

mmcshane commented Nov 9, 2023

If someone wants to do both at once, this should be an additional setting and not the default

This seems reasonable. I'll see if I can get this behavior in place unless anyone else wants to make an argument for the current approach (which always organizes imports)

@jhossbach jhossbach linked an issue Nov 10, 2023 that may be closed by this pull request
@mmcshane
Copy link
Contributor Author

After a little more thinking I'm torn on the issue of whether or not to format imports by default. @JaRoSchm is clearly correct that ruff does not format imports when invoked from the command line. But this plugin has always formatted imports when used as a pylsp formatter. The relevant configuration is pylsp.plugins.ruff.format and looking at the old implementation of run_ruff_format there has never been any way to even disable import formatting.

So the question is whether to remain compatible with existing behavior of this plugin and always sort imports or to do the less surprising thing for ruff users and make import sorting opt-in (or maybe don't provide it at all?). I don't see a way that we can do both. @jhossbach do you have an opinion here?

@mmcshane mmcshane force-pushed the mpm/ruff-format branch 3 times, most recently from cb6bb60 to 077f739 Compare November 10, 2023 17:10
@jhossbach
Copy link
Member

So, if I understand astral-sh/ruff#8232 correctly the idea will be to have a command like ruff fix that does the same as ruff format && ruff check --fix, correct? Then python-lsp-ruff will run ruff fix --extend-select <list of format codes> when textDocument/formatting is invoked?
I think giving isort to the list of error codes explicitly rather than implicitly will avoid confusion, so let's leave it out as the default and we give an example in the README for how to use with isort.

@mmcshane
Copy link
Contributor Author

I believe this PR now has the behavior described - the second pass through the document will only occur if the format config parameter is given and will only include the checks indicated therein.

I just want to make clear that users using python-lsp-ruff today are getting import sorting and are not getting code formatting. This PR inverts that behavior - users will get code formatting and will not (by default) get import sorting. IMO this is fine and would have to happen at some point but I want to draw attention to the change as there may be some confusion among users.

Adds the Subcommand enum to indicate which `ruff` subcommand should be
executed by `run_ruff`. At this time, only `check` and `format` are
supported. As different subcommands support different parameters,
argument generation is delegated based on the specific subcommand value.

The `ruff format` subcommand does not currently organize imports and
there does not appear to be a way to convince it to do so. Until a
unified command exists the approach taken here is to format and then
make a second run of `ruff check` that _only_ performs import
formatting.
Codes listed in this setting should be included in fixes performed as
part of a formatting pass.
@jhossbach
Copy link
Member

LGTM, merging.

@jhossbach jhossbach merged commit 9ff905c into python-lsp:main Nov 25, 2023
5 checks passed
@GaetanLepage
Copy link

Thank you for this change !
Is a release planned anytime soon ?

@jhossbach
Copy link
Member

jhossbach commented Nov 25, 2023

I plan to wrap up some of the other issues as well as updating the README, but I expect to create a new release today.

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.

Support ruff format
4 participants