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

Added more LSP support for Python by adding ruff-lsp support. #3926

Merged
merged 7 commits into from
Feb 19, 2023

Conversation

frejanordsiek
Copy link
Contributor

As it says on the tin, adds support for ruff-lsp for Python (https://github.com/charliermarsh/ruff-lsp) including adding it to the documentation (not completely sure I got it right).

ruff-lsp was brought up in Issue #3876 where others have been working on it and had some success in getting it working.

This implementation provides customization options for every setting ruff-lsp has but one (showNotification since I am not sure what all values it supports). It is heavily based off of clients/lsp-pylsp.el, with some ideas from the other ones. Note, unlike the version from Nikolai Prokoschenko (@rassie) in Issue #3876, this does not have poetry support.

I set its priority to -2, putting it behind pylsp which most people seem to use and prefer (no surprises for users).

@github-actions github-actions bot added client One or more of lsp-mode language clients documentation labels Jan 23, 2023
:path lsp-ruff-lsp-ruff-path
:interpreter (vector lsp-ruff-lsp-python-path)
:organizeImports (if lsp-ruff-lsp-advertize-organize-imports t :json-false)
:fixAll (if lsp-ruff-lsp-advertize-fix-all t :json-false))))))
Copy link
Member

Choose a reason for hiding this comment

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

nit: lsp-json-bool

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't realize that existed (not a surprise). Thank you. I have changed the code to use it.

:group 'lsp-ruff-lsp)


(lsp-register-client
Copy link
Member

Choose a reason for hiding this comment

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

shouldn't that be add-on? = t?

Copy link

Choose a reason for hiding this comment

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

I'd strongly suggest that too, ruff is more akin to ESLint, as it's normally used in combination with a "proper" language server.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know. Reading the FAQ, it seems like add-on? being t means load in parallel to other LSPs at the same priority? Is compatibility something to worry about? Only a few of the client modules use it:

  • lsp-eslint.el
  • lsp-emmet.el
  • lsp-sorbet.el
  • lsp-angular.el
  • lsp-camel.el

None of the python ones do at present (though, of course, that doesn't mean that they shouldn't).

So basically, I don't know enough about the option to know.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just saw @rassie explanation of what add-on? means and why (posted while I was writing my previous response) and I think I understand the reasoning now. Yes, this is a good fit for :add-on?. So, I have set it to t in a new commit.

Copy link

Choose a reason for hiding this comment

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

Ruff doesn't provide anything you'd like a language server to have, e.g. code navigation, completion etc., it's just a linter. So running it standalone is usually not what you want, but pairing it with e.g. pylsp is extremely natural -- you'd have two servers running, both providing code actions and diagnostics at the same time (e.g. pylsp would provide mypy diagnostics, while ruff would be linting). It's exactly what we want and need (like I said, it's basically the same situation as with eslint). We haven't seen addon LSPs with Python yet, since pylint and pyflakes and flake8 and all the other linters have been typically used as plugins for pylsp, but ruff is mostly used standalone (but there is a plugin for pylsp too).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So that is why the other python LSPs have not done it. Makes sense. Speaking of, the option to enable ruff for pylsp does need to have its customization options added but that is for another day.

Thank you for the explanations.

@frejanordsiek
Copy link
Contributor Author

I rebased the PR, added the showNotifications option, added the missing values for the logLevel option, and added the missing importStrategy option (it is declared in the ruff-lsp code as well as its vscode plugin, but not in ruff-lsp's README).

@frejanordsiek
Copy link
Contributor Author

I do not know why the check on macos-latest with emacs 26.3 is failing

@nasyxx
Copy link
Contributor

nasyxx commented Feb 18, 2023

Any update for this? Will it merge to the main branch soon?

@yyoncho yyoncho merged commit e1bdae2 into emacs-lsp:master Feb 19, 2023
@yyoncho
Copy link
Member

yyoncho commented Feb 19, 2023

@frejanordsiek thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client One or more of lsp-mode language clients documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants