treewide: remove optional builtins prefixes from prelude functions#444432
Conversation
nixpkgs-ci
Bot
added
10.rebuild-linux: 1-10
nixpkgs-ci
Bot
removed
6.topic: module system
|
These did just get documented. NixOS/nix#14015 |
|
@trueNAHO I would like to merge these changes as follows:
As this patch was tool-generated (thank you for that!) I think this will be easy. My rationale is that each of these has a different risk profile and audience that might care. I feel on solid ground with understanding the impact in the first three, but much less so in the last. |
philiptaron
left a comment
philiptaron
left a comment
There was a problem hiding this comment.
Looked through all but the nixos changes.
| @@ -76,7 +76,7 @@ let | |||
| else | |||
| base; | |||
|
|
|||
| # Implements the builtins.toString interface. | |||
| # Implements the toString interface. | |||
There was a problem hiding this comment.
this one is questionable
There was a problem hiding this comment.
this one is questionable
Seems good to me, although the comment itself could be removed since it is somewhat self-explanatory.
| # what arguments were effectively passed *to* mkDerivation as opposed to | ||
| # builtins.derivation (by mkDerivation). For example, stdenv.mkDerivation | ||
| # derivation (by mkDerivation). For example, stdenv.mkDerivation | ||
| # accepts an `env` attribute set which is postprocessed before being passed to | ||
| # builtins.derivation. This can lead to evaluation failures, if we assume | ||
| # derivation. This can lead to evaluation failures, if we assume | ||
| # that drvAttrs is equivalent to the arguments passed to mkDerivation. | ||
| # See https://github.com/NixOS/nixpkgs/issues/269539. |
There was a problem hiding this comment.
These changes make the documentation worse and I would like them reverted
There was a problem hiding this comment.
Would it not be better to code format instead of reverting them:
-# derivation (by mkDerivation). For example, stdenv.mkDerivation
+# `derivation` (by mkDerivation). For example, stdenv.mkDerivation
# accepts an `env` attribute set which is postprocessed before being passed to
-# derivation. This can lead to evaluation failures, if we assume
+# `derivation`. This can lead to evaluation failures, if we assumeThe comment already code formats env.
|
These "whoops, this changed a comment, but it probably shouldn't" comments indicate to me that replaces like this should really be done AST-aware, for example with ast-grep. |
While I agree in general, having read through the changes, they're small enough and occur in few enough comments that in this case a split PR is quite mergeable. |
|
I don't get it, does this change have any benefits? |
It makes the number of attrset lookups go down by about 20k (https://github.com/NixOS/nixpkgs/actions/runs/17869102318?pr=444432). Super small change in total, but not nothing. In addition to that, having Nixpkgs code sometimes qualify |
Thanks for pointing that out. Although I have always wondered what the prelude functions are, I never bothered looking it up (presumably in the Nix code base). I have extended the script to remove the
Yes. However, since inspecting subsystems can be done with something like
Since the diff is relatively small and easy (only
Aside from the small 0.49% CPU time reduction, this is primarily motivated by the fact that this ensures a consistent code style and that |
|
I want to be clear @trueNAHO -- I won't merge this PR in the present state, since I don't feel authority over all the areas it touches, and I've been burned merging "safe" PRs in the The point isn't to have pure, perfect commits. The point is to be able to merge. Currently this PR has zero people willing to merge it. I'm offering to move that to one, at the cost of splitting the PR. Also, please honor my requests to revert the documentation and comments. In my view, the comments are more clear the way they were before. |
Remove optional builtins prefixes from prelude functions by running:
builtins=(
abort
baseNameOf
break
derivation
derivationStrict
dirOf
false
fetchGit
fetchMercurial
fetchTarball
fetchTree
fromTOML
import
isNull
map
null
placeholder
removeAttrs
scopedImport
throw
toString
true
)
fd --type file --exec-batch sed --in-place --regexp-extended "
s/\<builtins\.($(
printf '%s\n' "${builtins[@]}" |
paste --delimiter '|' --serial -
))\>/\1/g
"
nix fmt
Got it. I split this PR into the following smaller PRs:
This PR was repurposed as the
Much appreciated :)
Do you mean reverting documentation and comments in general, or only the specifically mentioned ones 1 2? I have manually post-reverted those in commit 048ac74 ("pkgs: add builtins prefixes for prelude functions to improve clarity") from #447404. |
|
Successfully created backport PR for |
|
Is there a style guide that prompted this? |
Not AFAIK. I opened #449123 to discuss this. |
6 participants
Related PRs intentionally split up to simplify review and merging:
Pinging @Aleksanaa because (IIRC) I have seen plenty treewide cleanups from her.
Changelog
v3: dec908b
doc,lib,pkgs, andnixoschanges into smaller PRs.v2: e4421a5
Rebase on top of commit 9b2c4d6 ("kdePackages.plasma-wayland-protocols: 1.18.0 -> 1.19.0 (kdePackages.plasma-wayland-protocols: 1.18.0 -> 1.19.0 #444047)").
For convenience, here are the relevant rebase changes.
v1: 72bf0b9
v0: c65f82b
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.