statix icon indicating copy to clipboard operation
statix copied to clipboard

Add a repeated keys lint

Open viraptor opened this issue 2 years ago • 2 comments

Check for repeated keys in attrsets. For example:

foo.bar1 = 1;
foo.bar2 = 2;
foo.bar3 = 3;

should recommend creating a

foo = { ... }

The threshold for flagging is 3 repeats.

Since the warning doesn't really belong to a specific key, the location points at the whole AttrSet. It's possible to make it better with a precise suggestion, but it would be a lot of code. Maybe a todo for the future.

viraptor avatar Jun 30 '22 10:06 viraptor

Thanks for the PR and sorry for the tardy reply! Just a couple of comments:

  • could you add unit tests for the lint? the process is as follows:
    • install insta, this is the snapshot testing tool we use
    • add a test file to bin/tests/data with the same name as the lint preferably, you can take a look at existing data for reference
    • run cargo insta test, and accept new snapshots after confirming that the expected output is seen
  • do you think the threshold should be configurable using the statix.toml file? if so, we should add such an option, you could add it to this PR, or a separate one if you'd like!

oppiliappan avatar Jul 10 '22 13:07 oppiliappan

do you think the threshold should be configurable using the statix.toml file?

There's no existing usage of something like that, right? I'd have to pass it to session like nix_version is done currently?

viraptor avatar Jul 16 '22 03:07 viraptor

this has been merged into master: ea0e5ee38eb6e3300f9cf0eba93793b8b29dfd01, don't see why github fails to detect and mark the PR. thanks for the contribution.

There's no existing usage of something like that, right? I'd have to pass it to session like nix_version is done currently?

you are right, but i think 3 is a reasonable default for now, so i'll leave it at that.

oppiliappan avatar Jan 08 '23 09:01 oppiliappan

I feel that this is often undesirable in NixOS/nix-darwin/home-manager configurations, where you have lots of separate services.* and programs.* definitions; those are more namespace tags for sets of options than they are part of a coherent hierarchy, so I'm not sure services = { ... }; is really better. I do try to avoid this style outside of that kind of thing, though; maybe a special case or two wouldn't go amiss here? This lint accounts for 100% of the complaints statix has about my personal configuration flake.

emilazy avatar Jun 18 '23 09:06 emilazy