-
-
Notifications
You must be signed in to change notification settings - Fork 14k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
lib.strings: deprecate lib.strings.{head,tail} #313144
base: master
Are you sure you want to change the base?
Conversation
We should probably |
Yep, that's very fair, you're probably right... I think after a period it'd be nice to fully remove them though since they still show up in e.g. tabcompletion in the REPL otherwise.. I figured in this case it might make sense to be a bit more bold than usual for deprecation since well... the existing functions don't even work on strings (so chances are pretty slim anyone depends on importing these from |
You'd be surprised what people do, especially accidentally, since nix lacks robust static analysis |
92895c9
to
4a3df5b
Compare
I updated the PR to warn instead of fully remove the functions, to offer a grace period for people to correct any eventual use of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just some relatively minor comments, but otherwise this looks good!
lib/strings.nix
Outdated
head_tail_warn_msg = name: | ||
'' | ||
lib.strings.${name} only supports lists; use lib.lists.${name} instead. | ||
The lib.strings.${name} alias is deprecated and will be removed after the NixOS 2024.11 release. | ||
''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Imo it's a bit overkill to abstract such an error message, and as implemented this actually slows down lib
evaluation a little bit, because of the extra //
used. I think it would be fine to just inline the error twice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, I pulled it out because the main attrset is rec
and already uses head
/tail
a few times but we can change the couple uses to explicit builtins.{head,tail}
in the file
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I moved it into the main rec {}
attrset with exports and changed occurrences of head
/tail
in the file to use builtins.{head,tail}
explicitly
4a3df5b
to
2733fca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a minor comment, looking good after that!
lib/strings.nix
Outdated
@@ -85,7 +94,7 @@ rec { | |||
list: | |||
if list == [] || length list == 1 | |||
then list | |||
else tail (lib.concatMap (x: [separator x]) list); | |||
else builtins.tail (lib.concatMap (x: [separator x]) list); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of doing this, you can add
inherit (lib) head tail;
to the top let in
(the preference should be to get symbols from lib
, because it's more flexible)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The problem with that is the top-level let-in
is above the rec { }
attrset where we define the deprecated head
and tail
with warnings... so any unqualified references will refer to those and throw warnings.
(That's why at first I had pulled those deprecated attributes out into a separate attrset that was merged in with //
before...)
I can access the functions from lib
instead of builtins
though if that's preferred
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moved to using lib.{head,tail}
instead of builtins.{head,tail}
as requested. As mentioned I don't see how a let-inherit
would help here since the head
/tail
with warnings in the returned recursive attrset would shadow them. Please let me know if I'm missing something, or if there's a different approach you would prefer...
These are aliases for builtins.{head,tail}, which only operate on lists and error if you try to feed them strings. They're already exposed under `lib` and `lib.lists`, which are less misleading names.
2733fca
to
fe5b0ed
Compare
These operate on lists and error if you try to call them on strings... and they're already exposed under
lib
andlib.lists
Description of changes
lib.strings.head
is no longer an alias forbuiltins.head
, ditto fortail
Things done
I checked (somewhat sloppily) for references with
rg lib.strings.head
,rg '\(lib.strings\) .*head
,rg lib.strings.tail
,rg \(lib.strings\) .*tail
and didn't notice any. I checked that<nixpkgs>
and<nixpkgs/nixos>
evaluate (on my system/with my config) as wellnix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-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.