plugins/none-ls: refactor using `mkSourcePlugin` #1853

MattSturgeon · 2024-07-11T18:10:04Z

This wasn't possible before because we were using IFD, which apparently cannot be used to evaluate module imports.

Now we can use a "mkModule" style helper, allowing the implementation to be simplified overall. This should also simplify future refactoring, such as an rfc42 "withArgs" equivalent.

I was able to clean the test up a little too, now we don't need to list all "unpackaged" sources in the test; only sources we want to disable from being tested.

MattSturgeon · 2024-07-11T21:42:01Z

plugins/none-ls/_mk-source-plugin.nix

+      '';
+    }
+    # Only declare a package option if a package is required
+    // lib.optionalAttrs (packaged ? ${sourceName} || lib.elem sourceName unpackaged) {


@traxys can you clarify why it is useful to have a separate unpackaged list?

If we made packaged into a nullable list, none of the implementation (below) would have to change, but this line could be simplified:

Suggested change

// lib.optionalAttrs (packaged ? ${sourceName} || lib.elem sourceName unpackaged) {

// lib.optionalAttrs (packaged ? ${sourceName}) {

If you just like the list syntax and not needing to spam null repeatedly, we can use genAttrs:

packaged = { inherit (pkgs) actionlint alejandra # etc ; } # Unpackaged // genAttrs [ "blade_formatter" "bsfmt" # etc ] (_: null);

(I agree the package or null or boolean we had before was clunky, so I see the value in the separate noPackage list).

(I've pushed up a change implementing this, let me know if you'd rather I drop/revert it)

I don't have any issues with that, it was just simpler to write

traxys

Some nits/questions, but globally nothing much to say

traxys · 2024-07-13T17:14:52Z

tests/test-sources/plugins/none-ls.nix

+                [
+                  # As of 2024-03-22, pkgs.d2 is broken
+                  # TODO: re-enable this test when fixed
+                  "d2_fmt"
+                  # TODO: can this be re-enabled?
+                  "yamlfix"
+                ]
+                ++ (lib.optionals (pkgs.stdenv.isDarwin && pkgs.stdenv.isx86_64) [
+                  # As of 2024-03-27, pkgs.graalvm-ce (a dependency of pkgs.clj-kondo) is broken on x86_64-darwin
+                  # TODO: re-enable this test when fixed
+                  "clj_kondo"
+                ])
+                ++ (lib.optionals pkgs.stdenv.isDarwin [
+                  # As of 2024-05-22, python311Packages.k5test (one of ansible-lint's dependenvies) is broken on darwin
+                  # TODO: re-enable this test when fixed
+                  "ansible_lint"
+                  "clazy"
+                  "gdformat"
+                  "gdlint"
+                  "haml_lint"
+                  # As of 2024-06-29, pkgs.rubyfmt is broken on darwin
+                  # TODO: re-enable this test when fixed
+                  "rubyfmt"
+                  "verilator"
+                  "verible_verilog_format"
+                ])
+                ++ (lib.optionals pkgs.stdenv.isAarch64 [
+                  "semgrep"
+                  "smlfmt"
+                  # As of 2024-03-11, swift-format is broken on aarch64
+                  # TODO: re-enable this test when fixed
+                  "swift_format"
+                ]);


Have you checked that all the packages marked as broken are still broken ?

No I haven't. Is the best way to just empty the list and see what buildbot does?

Yeah I think... We can punt that to another PR

traxys · 2024-07-13T17:17:14Z

plugins/none-ls/sources.nix

+    in
+    lib.mkIf cfg.enable {
+      # Enable gitsigns if the gitsigns source is enabled
+      plugins.gitsigns.enable = lib.mkIf gitsignsEnabled true;


Do you think there is a way to have a warning if gitsigns was not already enabled? I think we will end up with an infinite recursion, but maybe you know a way to to that?
If we can add a warning we could remove this magic in 24.11

I think this is possible by checking the options.plugins.gitsigns.enable.definitions length.

If the definitions only has one element, then that's our mkIf. If there's multiple definitions then the user has defined it themselves.

Would we also want the warning to check if the final merged value would be true as well?

While I think it's possible, I don't really want to tackle it in this PR.

Yeah no problem

traxys · 2024-07-13T17:18:39Z

plugins/none-ls/_mk-source-plugin.nix

+}:
+let
+  inherit (import ./packages.nix pkgs) packaged;
+  pkg = packaged.${sourceName};


Lazy evaluation is nice :D

plugins/none-ls/_mk-source-plugin.nix

traxys · 2024-07-13T17:21:18Z

plugins/none-ls/_mk-source-plugin.nix

+  options.plugins.none-ls.sources.${sourceType}.${sourceName} =
+    {
+      enable = lib.mkEnableOption "the ${sourceName} ${sourceType} source for none-ls";
+      withArgs = helpers.mkNullOrOption helpers.nixvimTypes.strLua ''


It should be straighforward (in another PR) to have structured options here no? We'd need some kind of coercion from string to rawLua though

Agreed. #1705 is tracking the idea.

Introduce `_mk-source-plugin.nix`, which returns a module handling a specific none-ls source plugin. This wasn't possible previously due to IFD conflicting with evaluating module imports. Adjusted the test to use the `options` provided via module args, and cleaned up some stuff allowing "unpackaged" sources to not be listed again in the test file.

We can just make `packaged` nullable again.

traxys

CI fail seems unrelated

MattSturgeon · 2024-07-13T19:50:27Z

@Mergifyio queue

mergify · 2024-07-13T19:50:39Z

queue

🛑 The pull request has been removed from the queue `default`

The queue conditions cannot be satisfied due to failing checks.

You can take a look at Queue: Embarked in merge queue check runs for more details.

In case of a failure due to a flaky test, you should first retrigger the CI.
Then, re-embark the pull request into the merge queue by posting the comment
@mergifyio refresh on the pull request.

MattSturgeon · 2024-07-13T19:55:01Z

@Mergifyio queue

mergify · 2024-07-13T19:55:04Z

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 06a44e9

MattSturgeon force-pushed the none-ls_refactor branch from 4dae6fb to 7f22752 Compare July 11, 2024 21:02

MattSturgeon marked this pull request as ready for review July 11, 2024 21:13

MattSturgeon requested a review from a team July 11, 2024 21:15

MattSturgeon commented Jul 11, 2024

View reviewed changes

traxys reviewed Jul 13, 2024

View reviewed changes

MattSturgeon force-pushed the none-ls_refactor branch from 56277a0 to 7525906 Compare July 13, 2024 19:12

MattSturgeon added 2 commits July 13, 2024 20:20

plugins/none-ls: combine packaged+unpackaged lists

4f3cd9f

We can just make `packaged` nullable again.

plugins/none-ls: rename servers.nix to sources.nix

06a44e9

MattSturgeon force-pushed the none-ls_refactor branch from 7525906 to 06a44e9 Compare July 13, 2024 19:22

MattSturgeon requested a review from traxys July 13, 2024 19:24

traxys approved these changes Jul 13, 2024

View reviewed changes

mergify bot merged commit 06a44e9 into nix-community:main Jul 13, 2024
77 checks passed

mergify bot temporarily deployed to github-pages July 13, 2024 19:58 Inactive

MattSturgeon deleted the none-ls_refactor branch July 13, 2024 20:11

MattSturgeon mentioned this pull request Jul 15, 2024

none-ls: updating + refactoring #1705

Open

23 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

plugins/none-ls: refactor using `mkSourcePlugin` #1853

plugins/none-ls: refactor using `mkSourcePlugin` #1853

MattSturgeon commented Jul 11, 2024 •

edited

Loading

MattSturgeon Jul 11, 2024 •

edited

Loading

MattSturgeon Jul 11, 2024

traxys Jul 13, 2024

traxys left a comment

traxys Jul 13, 2024

MattSturgeon Jul 13, 2024

traxys Jul 13, 2024

traxys Jul 13, 2024

MattSturgeon Jul 13, 2024

MattSturgeon Jul 13, 2024

traxys Jul 13, 2024

traxys Jul 13, 2024

traxys Jul 13, 2024

MattSturgeon Jul 13, 2024

traxys left a comment

MattSturgeon commented Jul 13, 2024

mergify bot commented Jul 13, 2024 •

edited

Loading

MattSturgeon commented Jul 13, 2024

mergify bot commented Jul 13, 2024 •

edited

Loading

	// lib.optionalAttrs (packaged ? ${sourceName} \|\| lib.elem sourceName unpackaged) {
	// lib.optionalAttrs (packaged ? ${sourceName}) {

plugins/none-ls: refactor using mkSourcePlugin #1853

plugins/none-ls: refactor using mkSourcePlugin #1853

Conversation

MattSturgeon commented Jul 11, 2024 • edited Loading

MattSturgeon Jul 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

traxys left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

traxys left a comment

Choose a reason for hiding this comment

MattSturgeon commented Jul 13, 2024

mergify bot commented Jul 13, 2024 • edited Loading

🛑 The pull request has been removed from the queue default

MattSturgeon commented Jul 13, 2024

mergify bot commented Jul 13, 2024 • edited Loading

✅ The pull request has been merged automatically

plugins/none-ls: refactor using `mkSourcePlugin` #1853

plugins/none-ls: refactor using `mkSourcePlugin` #1853

MattSturgeon commented Jul 11, 2024 •

edited

Loading

MattSturgeon Jul 11, 2024 •

edited

Loading

mergify bot commented Jul 13, 2024 •

edited

Loading

🛑 The pull request has been removed from the queue `default`

mergify bot commented Jul 13, 2024 •

edited

Loading