Skip to content
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

treewide: Unquote most URLs #46378

Closed
wants to merge 1 commit into from
Closed

treewide: Unquote most URLs #46378

wants to merge 1 commit into from

Conversation

dasJ
Copy link
Member

@dasJ dasJ commented Sep 8, 2018

Yes, I'm serious

Motivation for this change

Quoted URLs are strings instead of URLs. When using another package as example when writing one of my first packages, it got rejected because URLs shouldn't be quoted.
This PR removes quotes from URLs so nobody uses them as example as I did back then.
I excluded all URLs which contain one of these characters: $#";(

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Fits CONTRIBUTING.md.

@infinisil
Copy link
Member

People intend to move in the opposite direction, see #27809 (comment)

@dasJ
Copy link
Member Author

dasJ commented Sep 8, 2018

Oh no

@dasJ
Copy link
Member Author

dasJ commented Sep 8, 2018

I mean I could just reverse the regexp…

@vcunat
Copy link
Member

vcunat commented Sep 8, 2018

I wouldn't hurry with such mass replacements. I personally can't see why it should make a significant difference if you use one way or another one.

@dasJ
Copy link
Member Author

dasJ commented Sep 8, 2018

@vcunat The main reason in my opinion is consistency. Having all packages follow the same pattern is a lot less confusing for new users. Of course there is an almost infinite amount of ways you can package software, but IMO we should have some common ground for some parts. Maybe this PR isn't the proper location to discuss this and this is probably not the best time as well, as a new release is pretty close and there is more important stuff to do.
But these inconsistencies and "we'll refactor that one day"-things tend to add up and I always try to fix/refactor/… them ASAP.

@vcunat
Copy link
Member

vcunat commented Sep 8, 2018

Perhaps, but first we need a clear consensus that one of the two variants is preferred ;-)

@dasJ
Copy link
Member Author

dasJ commented Sep 8, 2018

Yes, I agree with you. What do you think is the proper place for such a discussion? Discourse? IRC? Private Mails? This PR? A RFC?

@vcunat
Copy link
Member

vcunat commented Sep 8, 2018

This thread isn't a bad start, IMHO. RFC is another option. We could link to that thread from other places if it seems there are too little eyes.

Consistency: note that we have URLs inside fetchurl and similar calls very commonly, and there we often want to interpolate some variables, and that can't be done in the unquoted style ATM.

@dasJ
Copy link
Member Author

dasJ commented Sep 8, 2018

Yes, I also found that the url type fails for the characters I mentioned above: $#;(. While $ is mostly used when using variables in a URL (which is a legitimate thing to do of course), #, ;, and ( are used as well. So these URLs would have to be strings (or am I just missing some escaping rules?) meaning that using the URL type will always include inconsistencies.

I haven't found other places where I can link this thread yet, but I'll keep looking :)

@ryantm ryantm removed the request for review from basvandijk September 8, 2018 20:55
@oxij
Copy link
Member

oxij commented Sep 9, 2018 via email

@7c6f434c
Copy link
Member

And we do need quoted URLs for src handling (putting ${version} inside the URL string is a good idea from the point of view of avoiding sily mistakes), so full unification means either quoted URLs or rejecting useful tools.

@edolstra
Copy link
Member

While I'm favor of not quoting URLs, this PR causes a lot of code churn for a minor syntactic cleanup, which I think is to be avoided.

(BTW, I don't agree with the sentiment that "People [who?] intend to move in the opposite direction".)

@oxij
Copy link
Member

oxij commented Sep 10, 2018

@edolstra

People intend to move in the opposite direction, see #27809 (comment)
People [who?] intend to move in the opposite direction

Most people who commented in #27809, duh.

@infinisil
Copy link
Member

Closing this as most seem to be against this change (me included). Maybe eventually this could be enforced with some ofborg check to disallow unquoted URLs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants