-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
pkgs: refactor needless quoting of homepage meta attribute #27809
Conversation
A lot of packages are needlessly quoting the homepage meta attribute (about 1400, 22%), this commit refactors all of those instances.
I used this command to track down some links that shouldn't have been unquoted:
and fixed them. |
@@ -22,7 +22,6 @@ rec { | |||
]); | |||
|
|||
meta = with stdenv.lib; { | |||
homepage = ; |
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.
This package wasn't declaring any homepage anyways, so I removed the attribute.
You can use the following command to check evaluation:
|
@FRidh Thanks a lot, used it to track down the last few errors. |
Dammit, this is the opposite direction I was hoping we'd go 😦 Lots of us in the community think the magic URI/URL syntax in Nix is a wart and would like to move away from it, so I quote all URLs in my packages precisely because I want to make it easier for us to eventually ditch the special cases in the Nix grammar. The grammar is awkward today because it doesn't really parse full URIs and it can parse things as URIs that nobody intended to be. There's a Nix ticket on this but I can't find it right now. |
Here's some links: NixOS/nix#1017 NixOS/nix#836 NixOS/nix#1019 |
@copumpkin Didn't know about this, I also thought maybe this should get some more eyes before being merged. I don't really mind either way of doing it, as long as it's consistent, and since only 22% were using strings, I thought this would be the better way. I would totally be willing to undo this PR and change every other occurence of non-quoted URL's to quoted ones. |
I dunno, I suppose that now we have an RFC process, the right thing to do would be to put it to a vote to eliminate the magic syntax since it doesn't really buy us anything over strings, and complicates the grammar (and human model of syntax) considerably. Then as part of that, we'd mass-rewrite URLs to use quotes and slowly deprecate the syntax over the next few Nix releases. The whole "grassroots" thing I described above was probably not a very effective way of going about it 😄 |
@copumpkin Do you think this PR should be reverted for now then? Having 22% packages that do it 'right' is better than 0% I'd say :) |
FWIW, that's exactly my stance as well. Nixpkgs has had some issues in the past where the ambiguity between functions ( |
I also prefer to use strings instead of some special type. But currently that's what we have so I'm of the opinion we should stick with it until we've actually decided otherwise. So, indeed, go for a RFC :) |
|
If this URL syntax removed, nix will become a language, where id implementation uses minimal amount of symbols. |
From #34798. I see no policy behind this. What I see is @infinisil making a huge change to nixpkgs that goes backwards and then @FRidh merging it without discussion. And now you want people to make an RFC instead of just reverting this. |
I don't like this personally but we have #27809 as a precedent.
A lot of packages are needlessly quoting the homepage meta attribute
(about 1400, 22%), this commit refactors all of those instances.
Command used for refactoring (using ripgrep):
Notes: This excludes all the automatically generated files and the ones that contain an URL that wouldn't be valid unquoted (containing
${
/#
/(
), which obviously shouldn't be refactored.Motivation for this change
Of the 6280 instances of the homepage attribute (
rg 'homepage = .*' -g '!*-packages.nix' -g '!*-generated.nix' -l | wc -l
), there are 1396 instances where it's needlessly quoted (rg 'homepage = "[^\$\{#\(]*"' -g '!*-packages.nix' -g '!*-generated.nix' -l | wc -l
). This PR makes all of these uniform.Things done
Tested the changes with
(nix.useSandbox on NixOS,
or option
build-use-sandbox
innix.conf
on non-NixOS)
nix-shell -p nox --run "nox-review wip"
./result/bin/
)