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

languages/python: add ruff as lsp #575

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

Conversation

QuiNzX
Copy link

@QuiNzX QuiNzX commented Jan 22, 2025

In this PR I've added ruff as lsp.

Breaking change:
What has changed is that before cfg.lsp.server expected a single value but now it expects a list.
So that ruff can work in conjuction with basedpyright(or any other lsp) which is recommended according to the ruff docs.

I might going to add more granular options for the lsp if you like unless you have something else in mind that will do so. ruff lsp
This is my first nix pr so let me know if I made any mistakes!

EDIT: because of the toList function the breaking change isn't the case anymore for cfg.lsp.server .
Let me know if there is a better approach to it!
However the breaking change is still in the case of python.lsp.package which now requires each lsp server specified as an attribute set . On the bright side you can now set arguments for each lsp.
Like so:

package = {
  ruff = [lib.getExe pkgs.ruff "server"];
  basedpyright = [lib.getExe pkgs.basedpyright];
};
  • I have updated the [changelog] as per my changes
  • I have tested, and self-reviewed my code
  • Style and consistency
    • I ran Alejandra to format my code (nix fmt)
    • My code conforms to the [editorconfig] configuration of the project
    • My changes are consistent with the rest of the codebase
  • If new changes are particularly complex:
    • My code includes comments in particularly complex areas
    • I have added a section in the manual
    • (For breaking changes) I have included a migration guide
  • Package(s) built:
    • .#nix (default package)
    • .#maximal
    • .#docs-html (manual, must build)
  • Tested on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin

@QuiNzX QuiNzX requested a review from NotAShelf as a code owner January 22, 2025 00:18
github-actions bot pushed a commit that referenced this pull request Jan 22, 2025
Copy link

github-actions bot commented Jan 22, 2025

🚀 Live preview deployed from 4242640

View it here:

Debug Information

Triggered by: QuiNzX

HEAD at: feat-ruff

Reruns: 138

github-actions bot pushed a commit that referenced this pull request Jan 23, 2025
Copy link
Owner

@NotAShelf NotAShelf left a comment

Choose a reason for hiding this comment

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

Requested changes to remain consistent with the rest of the codebase, and to conform to our code styles.

To be honest, I don't quite like how this is handled. Not to say I can think of anything better at the moment, but this is entirely different from other language modules and might cause confusion.

I'll probably merge this after the requested changes are implemented, but I think a cleanup is due in the language module.


[QuiNzX](https://github.com/QuiNzX):

[ruff lsp]: (https://github.com/astral-sh/ruff)
Copy link
Owner

Choose a reason for hiding this comment

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

This reference is not used anywhere. You can remove it if you do not intend to refer to it as [ruff lsp] inline

default = defaultServer;
};

package = mkOption {
description = "python LSP server package, or the command to run as a list of strings";
example = ''[lib.getExe pkgs.jdt-language-server "-data" "~/.cache/jdtls/workspace"]'';
type = either package (listOf str);
default = servers.${cfg.lsp.server}.package;
type = lib.types.attrsOf (either package (listOf str));
Copy link
Owner

Choose a reason for hiding this comment

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

Please don't access lib inline

Suggested change
type = lib.types.attrsOf (either package (listOf str));
type = attrsOf (either package (listOf str));

where attrsOf is inherited from types in the top-level let in.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the feedback!
I've made the necessary changes. However I have yet to find a way to stay consistent with the rest of the codebase. I'm looking forward to the language module cleanup so that my diabolical change will be resolved.

@@ -268,7 +287,7 @@ in {

(mkIf cfg.lsp.enable {
vim.lsp.lspconfig.enable = true;
vim.lsp.lspconfig.sources.python-lsp = servers.${cfg.lsp.server}.lspConfig;
vim.lsp.lspconfig.sources = lib.genAttrs (lib.toList cfg.lsp.server) (name: servers.${name}.lspConfig);
Copy link
Owner

Choose a reason for hiding this comment

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

Move toList from an inherit at the top.

github-actions bot pushed a commit that referenced this pull request Jan 25, 2025
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.

3 participants