check-meta: fix hasNoMaintainers for missing maintainers attr
#357674
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
asymmetric
changed the title
hasNoMaintainers for missing maintainers attr
nix-owners
bot
requested review from
Artturin,
emilazy,
Ericson2314,
philiptaron,
reckenrode and
RossComputerGuy
emilazy
reviewed
Nov 20, 2024
There was a problem hiding this comment.
SGTM in principle. I assume length (attrs.meta.maintainers or [ ]) == 0 is probably bad for eval perf or something.
This will pick up things like the majority of trivial builders etc. that have no meta, I don’t know if that’s a concern. It might even complain about fetchers? That might be why it’s like that currently.
Could add a check if it's got
Yeah, maybe doing a |
@emilazy in my (probably naive) testing with hyperfine, they're ~equivalent in terms of speed:
Where:
|
mweinelt
reviewed
Nov 21, 2024
ofborg
bot
added
10.rebuild-darwin: 0
philiptaron
previously requested changes
Nov 23, 2024
Previously, when meta.maintainers wasn't defined, hasNoMaintainers would return false, since it relied on its presence to check for its length. Now, it returns true.
asymmetric
force-pushed
the
has-no-maintainer
branch
from
cb115fe to
e4945ac
Compare
|
Thanks all for the "code golfing" 😅 In my testing with hyperfine (see above), all solutions were basically equivalent in terms of run time (haven’t tested memory usage though). I still ended up implementing @zimbatm's suggestion, since it's much clearer than my first attempt, and avoids a function call. Still, we haven't addressed the issue of false positives with fetchers & co. @RossComputerGuy could you expand on what you mean by
|
|
I'm not sure what the CI failure is about... |
Yes, I think we're changing the previous semantics of 4def222. |
|
|
@RossComputerGuy thanks for the pointer (turns out the correct attr was
I believe hooks as well, since they're not FODs. |
|
At which point it would be more helpful to check for version or src to confirm it is a package. |
|
Changing the code to hasNoMaintainers = attrs: attrs ? src && attrs.meta.maintainers or [ ] == [ ];and building the
full output |
|
Are we slowly but surely circling back to checking for meta? |
|
I like the idea of weakening |
|
Problem is that (at least some) hooks have I don't understand why they do though, given how they're defined: 1 2. |
|
Well, perhaps hooks ought to have maintainers… |
|
@emilazy I mean I don't disagree (at all, in fact), but how to go about that? Seems like complexity explosion, from a 1-liner to an fuzzily defined social problem 😅 |
|
It’s an off‐by‐default warning, right? There’s a lot of stuff in the tree with no maintainers already. Warning about more of it only seems bad if it’s stuff that we don’t care about the maintainers for (like fetcher calls); for hooks it seems okay to me personally. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Previously, when
meta.maintainerswasn't defined,hasNoMaintainerswould return false, since it relied on its presence to check for its length.Now, it returns true, which seems more logical.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.