Add a repeated keys lint

oppiliappan / statix

lints and suggestions for the nix programming language

https://git.peppe.rs/languages/statix/about

MIT License

552 stars 21 forks source link

Add a repeated keys lint #47

Closed viraptor closed 1 year ago

viraptor commented 2 years ago

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.

oppiliappan commented 2 years ago

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!

viraptor commented 2 years ago

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?

oppiliappan commented 1 year ago

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.

emilazy commented 1 year ago

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.