Linting: Repeated or Grouped Keys - Polls

Development
1

About Linting,
And more precisely, about repeated or grouped keys :

Example

Repeated Keys

{
  a.b = true;
  a.c = true;
  a = { d = true; };
  a = {
    e.f = true;
    e.g = true;
  };
}

Grouped Keys

{
  a = {
    b = true;
    c = true;
    d = true;
    e = { 
      f = true;
      g = true;
    };
  };
}

Polls

Preferences

What’s your preference?

  • I always prefer Grouped Keys
  • I mostly prefer Grouped Keys
  • Neutral
  • I mostly prefer Repeated Keys
  • I always prefer Repeated Keys
0 voters

“Severity”

If the reporting was brought to LSPs (i.e. nixd) and linters (i.e. nixf-tidy, nixf-diagnose), what should the “severity” of Repeated Keys be?

  • Warning
  • Info
  • Hint
0 voters

Related

1 Like
2

The only reason I don’t have that lint enabled is because there’s no autofix. If Enable fixes for W20 repeated keys - group them into attribute sets by jakob1379 · Pull Request #163 · oppiliappan/statix · GitHub got merged I’d enable it right away.

3 Likes
3

For nixd, see Code Action: Attributes Grouping · Issue #831 · nix-community/nixd · GitHub, which proposes a Code Action (fix) related to the proposed “Repeated Key” lint raise (feat: repeated keys · Issue #842 · nix-community/nixd · GitHub)

1 Like
4

GitHub - oxalica/nil: NIx Language server, an incremental analysis assistant for writing in Nix. · GitHub has flatten and pack actions.

5

The reason I voted “I mostly prefer Grouped Keys” and not “I always prefer Grouped Keys” is because of the following example :

Strictly Repeated

{
  programs.a.enable = true;
  programs.b.enable = true;
  programs.c.enable = true;
  programs.d.enable = true;

  services.a.sync.enable = true;
}

Strictly Grouped

{
  programs = {
    a.enable = true;
    b.enable = true;
    c.enable = true;
    d.enable = true;
  };

  services.a.sync.enable = true;
}

Mostly Grouped, Few (Related ones) Repeated

{
  programs = {
    b.enable = true;
    c.enable = true;
    d.enable = true;
  };

  programs.a.enable = true;
  services.a.sync.enable = true;
}
3 Likes
6

I find that repeating keys is sometimes useful to group related config together.

1 Like
7

nixd also has an equivalent to nil’s flatten, but its attribute packing code action is not context aware (see Code Action: Attributes Packing - Context Awareness · Issue #830 · nix-community/nixd · GitHub) while nil’s is

I don’t know if nil’s pack code action take into its scope only the nearby attributes, or can also be set to work on the selection only, or if it takes the whole file

In which case those would be different from the “Repeated Keys” raise and the related code action

8

I also agree that repeating keys might sometimes be desirable, BUT also that most of the time, attribute grouping is instead (see Linting: Repeated or Grouped Keys - Polls - #5 by Malix)

9

When I started using statix in my code editor, at first I though that repeated_keys warning is useful. But when I went to my nixos config and tried to fix these warnings, I quickly realized, that I actually prefer repeated keys.
Here are the reasons:

  • Tbh, it’s more readable, having that prefix gives more context and makes it easier to understand what the option does. The alternative is to either keep that context in your head, or look up using lsp features (like vscode breadcrumbs), that’s why having that context right there in the same line is imo easier to read.
  • Inside files I try to place things in a way so that logically connected parts are near each other. Being forced to use grouped keys would make it impossible, as I would be forced to group keys that have a common prefix together. Eg., I have these lines in my configuration.nix:
    # Enable virtualization.
virtualisation.libvirtd.enable = true;
environment.variables.VAGRANT_DEFAULT_PROVIDER = "libvirt";
    Grouped keys would require me to split these two lines into two separated sections of the file, as I also have virtualisation.docker and environment.systemPackages in that same file.
  • I try to split my config into files, but my configuration.nix is still pretty big littered with some random tweaks. Enforcing grouped keys would increase indentation level by up to 5 in some lines, maybe more.
  • I’m using treesitter-context in neovim. Here’s an example of how it looks like:
    image
    image510×271 18.4 KB

    If I were to enforce grouped keys, this would result in home-manager.users.nix being split into 3 separate lines, and programs.firefox into 2 separate lines. This would waste vertical space in my editor. Though, that’s a bit specific problem, it’s not a problem in vscode where breadcrumbs are always in single line.

I do have a plan to switch to denful and put each logically-connected unit into separate file, in which case I might reconsider turning on repeated_keys warning. I think most of the issues would get alleviated this way.

5 Likes
10

If you need to write this, they probably need separate modules.

I always group.

2 Likes
11

Enforcing is only Warning, Hint or Info are suggestions

Also, grouping is when Repeated Keys are eliminated through nesting them into a common attribute set

Packing is when a flattened attributes chain is packed into nested attributes sets

If home-manager.users and programs are not repeated keys, there would be nothing to group

I also agree that I never want packing without grouping, even though I am a grouping enjoyer

12

Right, I forgot to mention that I also have home-manager.sharedModules, home-manager.users.root, and home-manager.users.nix.programs.ssh in that same file.

2 Likes
13

I almost always group, but sometimes I repeat keys because I don’t want to create another module just for 2 related attributes

1 Like
14

This is actually one of the reason to group, in my opinion

As soon as the keys’ names are longer than an indentation (nixfmt: 2 characters), grouping is more horizontal-space efficient (which also benefits parsing, but this is minor imo)

This is only true for the attributes that are being grouped, not the inner, already grouped attributes (see Linting: Repeated or Grouped Keys - Polls - #16 by MrQubo)

15

I very much agree with @MrQubo’s points. Repetition is actually good. It gives you valuable flexibility in where settings go in the file, and reduces the amount of structural complexity to keep track of when reading, as well as reducing the quantity of indentation overall.

3 Likes
16

As soon as the keys’ names are longer than an indentation (nixfmt: 2 characters), grouping is more horizontal-space efficient

Nope, that’s not always true.
Consider this example:

{
  aaa.bbb = 111;
  aaa.ccc = {
    ddd = 222;
    eee = 333;
  };
}

Packing aaa would result in:

{
  aaa = {
    bbb = 111;
    ccc = {
      ddd = 222;
      eee = 333;
    };
  };
}

In this case ddd = 222; got shifted right by 2 characters, and more horizontal space got used.

And that’s not made up. The increased indentation by 5 levels I was talking about is exactly like that. Grouping increases outer indentation levels, but the actual lines that get indented aren’t getting simplified by grouping at all.

17

Indeed, I just corrected my original reply :

However, such repeated / grouping mixing case is almost never seen in my personal configuration, I don’t know how common it can be

Strictly Repeated, it would be :

{
  aaa.bbb = 111;
  aaa.ccc.ddd = 222;
  aaa.ccc.eee = 333;
}

Which tales more horizontal, but less vertical space, albeit for the same vertical readability, it might actually better equates to

{
  aaa.bbb = 111;

  aaa.ccc.ddd = 222;
  aaa.ccc.eee = 333;
}

And except for collocating multiple related attributes, I never repeat nor mix repeating / grouping, and especially not for space efficiency, since it requires actively thinking about how much space would change

18

In my case it’s very common. I always group when the keys are already logically connected and together in the file. Often, keys that are more nested are more likely to be logically connected. So it’s often that I repeated keys at the top-level of attrset, but I exclusively group at more nested levels.

I’ll take this file from my configuration as an example. It’s for configuring code completion in neovim. Imo, it’s concise enough to be not worth splitting into multiple files.

completions.nix 
{ lib, ... }:

# TODO: Configure keybinds.
# The first click of Ctrl+n should complete the first entry on the list, not second. Like Ctrl+y, but without closing completion list.
# Bonus: The first click of Ctrl+p should complete the last entry on the list.
{
  programs.nixvim = {
    plugins.cmp = {
      enable = true;

      settings = {
        completion.completeopt = "menu,menuone,noinsert"; # Preselect first entry

        sources = [
          { name = "git"; }
          { name = "nvim_lsp"; }
          { name = "luasnip"; }
          { name = "path"; }
          { name = "buffer"; }
        ];

        mapping.__raw = "cmp.mapping.preset.insert()";

        snippet.expand = ''
          function(args)
            require('luasnip').lsp_expand(args.body)
          end
        '';
      };

      cmdline = lib.fix (self: {
        "/" = {
          mapping.__raw = "cmp.mapping.preset.cmdline()";
          sources = [
            { name = "buffer"; }
          ];
        };
        "?" = self."/"; # Not sure if that's needed?
        ":" = {
          mapping.__raw = "cmp.mapping.preset.cmdline()";
          sources = [
            { name = "path"; }
            { name = "cmdline"; }
          ];
        };
      });
    };

    plugins = {
      cmp-buffer.enable = true;
      cmp-cmdline.enable = true;
      cmp-git.enable = true;
      cmp-nvim-lsp.enable = true;
      cmp-path.enable = true;
      cmp_luasnip.enable = true;
    };

    plugins.lspkind.enable = true;
    plugins.lspkind.settings = {
      enable = true;
      cmp = {
        menu = {
          nvim_lsp = "[LSP]";
          nvim_lua = "[api]";
          path = "[path]";
          luasnip = "[snip]";
          buffer = "[buffer]";
        };
      };
      symbolMap = {
        Array = "[]";
        Boolean = "";
        Calendar = "";
        Class = "󰠱";
        Codeium = "";
        Color = "󰏘";
        Constant = "󰏿";
        Constructor = "";
        Copilot = "";
        Enum = "";
        EnumMember = "";
        Event = "";
        Field = "󰜢";
        File = "󰈙";
        Folder = "󰉋";
        Function = "󰊕";
        Interface = "";
        Keyword = "󰌋";
        Label = "󰜢";
        Method = "󰆧";
        Module = "";
        Namespace = "󰌗";
        Null = "󰟢";
        Number = "";
        Object = "󰅩";
        Operator = "󰆕";
        Package = "";
        Property = "";
        Reference = "󰈇";
        Snippet = "";
        String = "󰉿";
        Struct = "󰙅";
        TabNine = "";
        Table = "";
        Tag = "";
        Text = "󰊄";
        TypeParameter = "";
        Unit = "󰑭";
        Value = "󰎠";
        Variable = "󰀫";
        Watch = "󰥔";
      };
    };
  };
}

statix reports a warning here about repeated keys plugins. Noticed that in plugins.cmp I exclusively grouped where possible. If I were to pack plugins that would cause entire plugins.cmp definition to get intedented by one more level.

In the same file I also repeated plugins.lspkind:

    plugins.lspkind.enable = true;
    plugins.lspkind.settings = {

Again, packing would increase indentation level of plugins.lspkind.settings. One more thing, when actually reading that code, I don’t think my brain has to actually read plugins.lspkind twice, when the keys are repeated one after another like that, you can just pattern match in you head that plugins.lspkind is the common prefix (that only works if the keys aren’t too long).

Imo, while grouping can often be a good choice, some common sense should still be applied about when repeating is okay.

19

Neutral here, in the sense of finding that there are plenty of cases where one or the other of these will be the Much Better Choice for a given chunk of code.

Normal coder DRY instinct probably pushes most of us towards writing our prefixes only once, and this is still my default, but especially for NixOS module config sets I think this can lead to separating things by the wrong concerns, cutting along the X axis when you should have done it by Y instead. (Anyone know if there’s a name for this antipattern? Seems like there should be.)

There are plenty of cases where it is way less readable to put all the networking. stuff in one place, and all the services. stuff off somewhere else, and what you should really be doing to have good locality for the reader is stripe them so the related bits are together. Yes, you can also partition things off by file instead, but sometimes the configs are very very small and that gets a bit silly.

DRYing it up also hurts context for readability when you’re jumping into the middle of a big attrset and trying to figure out what’s going on. Editor tooling can help, but isn’t it easier to just have the entire path to the option you’re looking at on the same line? Etc, etc.

At least for me, taking it case-by-case absolutely makes sense.

2 Likes
20

To address question of severity, for me the best would be to implement some heuristics that would result in very small number of false positives. And in that case emit as warning.

One idea for heuristic I have in mind is to warn, if there are at least 4 repeated keys, without empty lines in between them (but non-empty are fine), and those 4 keys (including values) span no more than 15 lines.

1 Like