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

Support ruff format #53

Closed
JaRoSchm opened this issue Oct 20, 2023 · 16 comments · Fixed by #57
Closed

Support ruff format #53

JaRoSchm opened this issue Oct 20, 2023 · 16 comments · Fixed by #57

Comments

@JaRoSchm
Copy link

Hi, as far as I can see, the new ruff formatter comparable to black is not yet supported by this plugin. If I understand it correctly, the term "format" in the code of this repo relates to fixing errors. Would it be possible to add formatting (in the sense of black) as a new code action?

@JaRoSchm JaRoSchm changed the title Support ´ruff format´ Support ruff format Oct 20, 2023
@jhossbach
Copy link
Member

Hi, are you talking about this? https://github.com/astral-sh/ruff/blob/main/crates/ruff_python_formatter/README.md
I was not aware of this feature, but since it is in the Alpha state I would avoid adding this feature for now

@JaRoSchm
Copy link
Author

Yes, that's exactly what I mean. As this will probably come up again sooner or later, one could consider adding it behind an environment variable like ruff-lsp does it: https://github.com/astral-sh/ruff-lsp/blob/0bb19b4f56e2c99c7651ac391a964a1c8e59ffa5/ruff_lsp/server.py#L732

@JaRoSchm
Copy link
Author

Just a short note: The formatter was moved to beta and essentially production-ready today: https://astral.sh/blog/the-ruff-formatter

@haplo
Copy link

haplo commented Oct 25, 2023

I'm the maintainer for python-lsp-black, feel free to look and borrow its implementation to implement the formatting capabilities.

If nobody else is planning on working on it I will try to find time to implement it, I have been moving into a ruff-exclusive workflow and black was the last piece.

@jhossbach
Copy link
Member

@haplo I would greatly appreciate you working on it! I am not opposed to the idea but can currently not invest a lot of time into it

@haplo
Copy link

haplo commented Oct 30, 2023

@jhossbach I started work on this, but hit some issues and I'm not sure about the UX we should have.

The plugin already has a format document action to sort the imports, and the format setting is a list of strings (instead of a boolean to enable/disable formatting capabilities). I would repurpose the action to also run ruff format. I think this should be the default behavior, as I expect it to be the common case for ruff users. We can add another setting to allow disabling formatting (if someone wants to keep using black, for example). What should the setting be named if we cannot change format setting to keep backwards compatibility? formatEnabled defaulting to True? formatDisabled defaulting to False?

Another thing I discovered is that ruff format is not sorting imports, and it's done on purpose, see astral-sh/ruff#8087. This means that we will need to call ruff twice: once to fix imports, then again to invoke ruff format. Good news is that the ruff team is working on a combined command to format and sort imports, but it's still a WIP. I might wait until the new command is done to implement it.

One last thing: I think the plugin should disable pylsp_black, yapf and autopep8. python-lsp-black currently disables yapf and autopep8.

@jhossbach
Copy link
Member

I would repurpose the action to also run ruff format

Yes, that sounds like a good idea.

Another thing I discovered is that ruff format is not sorting imports

I am not entirely sure what change in the semantics can happen but I suppose they know what they are doing. If they come up with a solution that formats the code just like running black and isort would do (in addition to the code fixes I suppose?) I am happy.

I think the plugin should disable pylsp_black, yapf and autopep

You're right, also good idea, except it is called black and autopep8 in the configuration I think.

@betaboon
Copy link
Contributor

betaboon commented Nov 6, 2023

@haplo is there anything i could help you with, working on this?

@haplo
Copy link

haplo commented Nov 6, 2023

@haplo is there anything i could help you with, working on this?

I put it on pause waiting for the new ruff command that will do formatting and linting, so feel free to work on this.

Change is straightforward, just update run_ruff_format to include an extra run_ruff that calls ruff format after the imports have been sorted:

def run_ruff_format(
settings: PluginSettings,
document_path: str,
document_source: str,
) -> str:
fixable_codes = ["I"]
if settings.format:
fixable_codes.extend(settings.format)
extra_arguments = [
f"--fixable={','.join(fixable_codes)}",
]
result = run_ruff(
settings=settings,
document_path=document_path,
document_source=document_source,
fix=True,
extra_arguments=extra_arguments,
)
return result

Do add/update tests and documentation too.

@jhossbach I think as part of this work we should update the ruff dependency on python-lsp-ruff to the first ruff version with format support (0.1.2).

You can tag me in the pull request, would be happy to give it a try and review.

@mmcshane
Copy link
Contributor

mmcshane commented Nov 6, 2023

Change is straightforward, just update run_ruff_format to include an extra run_ruff that calls ruff format ...

Maybe not quite so straightforward as run_ruff doesn't appear to support subcommands AFAICT. But hopefully someone can point out where I've missed something?

@haplo
Copy link

haplo commented Nov 6, 2023

Change is straightforward, just update run_ruff_format to include an extra run_ruff that calls ruff format ...

Maybe not quite so straightforward as run_ruff doesn't appear to support subcommands AFAICT. But hopefully someone can point out where I've missed something?

You are right, some extra refactor work would be needed on run_ruff. All in all shouldn't be too hard, the existing test suite should help.

@jhossbach
Copy link
Member

You could modify this function and add an argument subcommand that defaults to "check" for ruff check ... unless ruff format is desired. Please check whether the other arguments generated work with it (e.g. ruff format --format=json etc.)

@Mithrandir2k18
Copy link

Sliding in here because I think my issue might be related to this discussion. I'm just trying to get ruff to format my files using the black ruleset they reimplemented. Is there a way to set this up yet? All I can get to work is isort.

@mmcshane
Copy link
Contributor

mmcshane commented Nov 9, 2023

@Mithrandir2k18 Yes this is your problem. If you're in the mood to test you can try installing python-lsp-ruff from git+https://github.com/mmcshane/python-lsp-ruff.git@mpm/ruff-format which is the PR above.

@jhossbach jhossbach linked a pull request Nov 10, 2023 that will close this issue
@Mithrandir2k18
Copy link

@Mithrandir2k18 Yes this is your problem. If you're in the mood to test you can try installing python-lsp-ruff from git+https://github.com/mmcshane/python-lsp-ruff.git@mpm/ruff-format which is the PR above.

works like a charm :) here's my pylsp config:

settings = {
	pylsp = {
		plugins = {
			ruff = {
				-- formatter + Linter + isort
				enabled = true,
				extendSelect = {
					-- pycodestyle
					"E",
					-- Pyflakes
					"F",
					-- pyupgrade
					"UP",
					-- flake8-bugbear
					"B",
					-- flake8-simplify
					"SIM",
					-- flake8-commas
					"COM",
					-- isort
					"I",
				},
				format = { "ALL" },
			},
                   ... -- switch off other parts of pylsp

Weirdly enough it doesn't fix missing trailing commas, which it should do, right?

@haplo
Copy link

haplo commented Nov 13, 2023

Weirdly enough it doesn't fix missing trailing commas, which it should do, right?

I believe it should, yes, same as Black does. Do check out the magic trailing comma docs in case that explains your situation.

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 a pull request may close this issue.

6 participants