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

[RFC 0004] Replace Unicode Quotes #4

Merged
merged 2 commits into from May 14, 2017
Merged

[RFC 0004] Replace Unicode Quotes #4

merged 2 commits into from May 14, 2017

Conversation

@layus
Copy link
Member

@layus layus commented Mar 19, 2017

Nix uses unicode glyphs to quote strings and paths in its output. This RFC
proposes to use only ASCII " and ' for quoting purposes in strings printed
during evaluation.

Rendered @ https://github.com/layus/rfcs/blob/master/rfcs/0004-replace-unicode-quotes.md

Copy link
Member

@zimbatm zimbatm left a comment

👍 overall. This RFC could be ready for general feedback quite quickly.

feature: replace-unicode-quotes
start-date: 2017-03-19
author: layus
co-authors: (find a buddy later to help our with the RFC)

This comment has been minimized.

@zimbatm

zimbatm Mar 19, 2017
Member

You can add me as a co-author if you want


1. _Correctness_: By removing preventively these characters, we will not have
to track the triggered issues separately. Unicode interact badly with
variable interpolation in bash and will create more issues if we keep them.

This comment has been minimized.

@zimbatm

zimbatm Mar 19, 2017
Member

This point could be more compelling with one or two examples

to track the triggered issues separately. Unicode interact badly with
variable interpolation in bash and will create more issues if we keep them.
2. _Compatibility_: Most terminal emulators do not recognise unicode quotes as
string delimiters. This makes string copy/paste from the terminal clumsy.

This comment has been minimized.

@zimbatm

zimbatm Mar 19, 2017
Member

Are there specific issues to point to? Are there specific fonts that's don't have the character defined?

Implementing this requires to replace every unicode quote glyph by an ASCII
character. This change needs only happen in strings intended to be part of
build logs or otherwise printed in the console. The automated change should not
alter comments nor documentation.

This comment has been minimized.

@zimbatm

zimbatm Mar 19, 2017
Member

It should also not modify derivation results

alter comments nor documentation.

After the change, using ASCII quotes should be enforced to maintain
consistency. This rule should be added to the dev manuals of nix and nixpkgs.

This comment has been minimized.

@zimbatm

zimbatm Mar 19, 2017
Member

Is nixpkgs part of the scope of this RFC or just nix?

# Unresolved questions
[unresolved]: #unresolved-questions

Should we also force ASCII quotes in pkgs meta fields and nixos optiosn decription ?

This comment has been minimized.

@zimbatm

zimbatm Mar 19, 2017
Member

typo: optiosn -> options

@layus
Copy link
Member Author

@layus layus commented Mar 23, 2017

@vcunat Could you help here by providing more references to your comment NixOS/nix#947 (comment) ? Is there some ML thread, IRC discussion or github comment I could refer to to make this point ?

@layus layus force-pushed the layus:master branch 4 times, most recently from d1bd452 to f86e610 Mar 23, 2017
@layus
Copy link
Member Author

@layus layus commented Mar 23, 2017

@zimbatm update to fix/improve on all you comments.

@zimbatm
Copy link
Member

@zimbatm zimbatm commented Mar 23, 2017

@layus thanks, do you mind to push further commits without rebasing? that helps keep me track of the changes over time.

Copy link
Member

@zimbatm zimbatm left a comment

I think it's ready for wider review. What do you think @layus ?

@layus layus force-pushed the layus:master branch from f86e610 to 8d44310 Mar 23, 2017
@layus
Copy link
Member Author

@layus layus commented Mar 23, 2017

@zimbatm You are the first guy I ever met to be against PR squashing (except me). Everything returned to normal, I restored the old commit and the update.

And yes, I think it is ready for wider approval :-)

@zimbatm
Copy link
Member

@zimbatm zimbatm commented Mar 23, 2017

Yeah, in this context I think it makes sense because there is only one logical commit produced at the end, which can be created with the "Squash and merge" button. A lot of the rebasing culture came before that button appeared.

@vcunat
Copy link
Member

@vcunat vcunat commented Mar 25, 2017

@layus: I can't see any comment of mine on that MR. What point do you want to support?

@vcunat
Copy link
Member

@vcunat vcunat commented Mar 25, 2017

AFAIK the main usage of non-ASCII (quotes) is currently in nix repo, probably mostly coming from /cc @edolstra. @domenkozar seemed to be confident that we should get rid of them. I've got personally no hard feelings about this. (These are really simple unicodes.) EDIT: but the reasoning seems well-founded, so I'm 👍 generally in the end.

@layus
Copy link
Member Author

@layus layus commented Mar 25, 2017

Yes, sorry, I meant @domenkozar. Got my brains messed up apparently :). Why did he(you) feel confident that we should get rid of unicode quotes ?

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Mar 26, 2017

Because our package manager is meant to work in many different environments and it's not worth the trouble getting terminal encoding working over some pragmatism and just using ASCII.

@spinus
Copy link
Member

@spinus spinus commented Mar 28, 2017

@domenkozar on a practical side, I think most distros which nix can run on, already use UTF-8. Even if not, I think (not sure 100%) despite the encoding you can have non-ascii, non-utf-8 characters in path anyway so this situation needs to be supported anyway, is that right?

@avnik
Copy link

@avnik avnik commented Mar 29, 2017

Upvote! basic package management, pkgs querying and early boot stages should be 7bit clean

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Mar 29, 2017

@spinus you could have other encoding than utf-8 for filesystem and terminal, then our utf-8 encoded prints would fail. We could detect what terminal encoding is used ($LANG) and use that for printing, hoping it would fix macOS. But then, why all the fuzz?

@spinus
Copy link
Member

@spinus spinus commented Mar 29, 2017

I would vote exactly for what you proposed with detecting $LANG.

But then, why all the fuzz?

I think that's good question and I assume that's why people started RFC. But as far as we have that we can document (in a central place) why certain decision was made (to not discuss it too often :-) )

@zimbatm
Copy link
Member

@zimbatm zimbatm commented Mar 29, 2017

Is it worth the added complexity though? Regardless of the $LANG discussion, it would still cause friction when copy-and-pasting on debug.

@Mic92
Mic92 approved these changes Mar 29, 2017
@garbas
Copy link

@garbas garbas commented Mar 29, 2017

👍 from my side, but I would like to know why @edolstra chose those characters in the first place. Maybe there is a reason that we are not aware of.

Another thing: Would this maybe break Hydra? The view where logs are collapsable, maybe somewhere else...

@benley
Copy link
Member

@benley benley commented Mar 29, 2017

The collapsible part of hydra's log viewer is already broken for unrelated reasons; see NixOS/hydra#240. This would not make it worse, as the feature does not rely on unicode quoting.

@teh
Copy link

@teh teh commented Apr 5, 2017

Did anyone volunteer for implementing this?

@vcunat
Copy link
Member

@vcunat vcunat commented Apr 6, 2017

Replacing them is relatively easy, but the IMO the largest blocker now is missing reaction from @edolstra.

@edolstra
Copy link
Member

@edolstra edolstra commented Apr 10, 2017

The main motivation - that some terminal emulators handle UTF-8 badly - doesn't seem very convincing to me. I mean, come on, it's 2017. If a terminal emulator can't handle UTF-8 today, it's time to fix or replace it. Nix is hardly the only program that does Unicode. FWIW, I've never had problems with this in Konsole.

However I don't feel super strongly about this.

@domenkozar
Copy link
Member

@domenkozar domenkozar commented Apr 10, 2017

@edolstra it does bring a good point, why doesn't it work on macOS?

@edolstra edolstra changed the title [RFC 004] Add Replace Unicode Quotes draft. [RFC 004] Replace Unicode Quotes Apr 10, 2017
@esoeylemez
Copy link

@esoeylemez esoeylemez commented Apr 11, 2017

Many programs in order to handle Unicode properly need to have at least LC_CTYPE set, which requires glibcLocales, which in turn requires 109 MiB of extra closure. While I do agree that everything should just be Unicode-ready these days, that's not the way it is. I'm in favour of switching to ASCII for now. Yes, "it's 2017", and we still suck at encodings.

That said, I don't run into any problems with Unicode quotes in practice. We're building our containers using Nix, but we're not actually adding Nix itself to them. And if we were, my first impulse would be to write a function that generates a narrower variant of glibcLocales, because most of the time only one or two locales are needed.

@vcunat
Copy link
Member

@vcunat vcunat commented Apr 11, 2017

@esoeylemez: the package has a parameter that you can override to the desired list of locales, and there's even an explicit option in nixos: http://nixos.org/nixos/manual/options.html#opt-i18n.supportedLocales

@zimbatm
Copy link
Member

@zimbatm zimbatm commented Apr 11, 2017

I think the main reason should be changed to being a (small) usability issue.

  1. When nix outputs a path, usually I copy, type vim and paste it. I have to make sure to not select the outer quotes because then it's not a valid path anymore. It means I have to be more precise in my selection.

  2. In some shells (like Hyper) the unicode quote is not a word delimiter. It means that in the following example:

cat <<FOO
a.   ‘0ff098i8dlr6frks67ik0kbc281c6j8lkb6v0y33iwqv45n233q3’
b.   '0ff098i8dlr6frks67ik0kbc281c6j8lkb6v0y33iwqv45n233q3'
FOO

(a) selects the outer quotes, (b) selects only the hash.

Arguably I could also use a better terminal :)

@esoeylemez
Copy link

@esoeylemez esoeylemez commented Apr 12, 2017

@vcunat: Thanks, I didn't know that.

All in all I'm still in favour of replacing them though. The UX issue @zimbatm mentioned has been irritating enough that I had actually made a pull request to replace the quotes in one specific location (when hash matching fails), because copying a hash is very awkward right now. It got rejected, because (understandably) it would make the quotes inconsistent.

My view is this: there doesn't seem to be any advantage of Unicode quotes other than aesthetics. But they do come at a practical expense. If the maintainers agree, I'm willing to do the actual work of replacing them everywhere, except perhaps in docs, where I believe the issues don't apply.

@danbst
Copy link

@danbst danbst commented Apr 12, 2017

@edolstra
Attaching a GIF on how double-click selecting works in urxvt with 1) unary quotes, 2) Nix quotes:
nix-select-hash

@layus
Copy link
Member Author

@layus layus commented Apr 12, 2017

@zimbatm I have the feeling that the discussion covered the important aspects, and that we reached a good understanding or the issue.

To summarize, this RFC proposes an impacting change to the codebase to get rid of annoyances and occasional bugs. Removing unicode quotes is the simplest way to fix these annoyances and potential new issues.

Not implementing this RFC means that we can keep these nice quotes. It would state our will to support unicode correctly in the nix ecosystem, by fixing issues, providing patches and writing manuals on how to type/enter these characters.

We all need to pick our fights, and I am ready to drop aesthetics and unicode integration to spare time for other tasks. This process itself is taking some energy :-).

Lookink at the discussion, people are mostly in favor of the RFC and no strong objections were raised. Some even proposed to implement it. Let's take a final decision, and start with another RFC, I see that there are more pending :-).

@shlevy
Copy link
Member

@shlevy shlevy commented Apr 12, 2017

Not sure what the final decision process is, but if it's a vote: thumbs up or thumbs down on this comment.

@benley
Copy link
Member

@benley benley commented Apr 12, 2017

One last thing I'll add: This proposal does not need to be a referendum on Unicode support in the Nix ecosystem in general. It's just about the console output of the commandline tools, right?

@zimbatm
Copy link
Member

@zimbatm zimbatm commented Apr 12, 2017

@esoeylemez
Copy link

@esoeylemez esoeylemez commented Apr 12, 2017

Great. I'd like to wait until Saturday, and if the number of upvotes on shlevy's comment is higher than the number of downvotes, I'll implement the change and send a pull request.

@vcunat
Copy link
Member

@vcunat vcunat commented Apr 12, 2017

I think there's a good consensus already, and the RFC has been open for almost a month, giving enough time to notice it, but we can wait a couple more days. EDIT: I don't connect this at all with reducing unicode support in nix(pkgs).

@zimbatm zimbatm changed the title [RFC 004] Replace Unicode Quotes [RFC 0004] Replace Unicode Quotes Apr 13, 2017
@aneeshusa
Copy link

@aneeshusa aneeshusa commented May 5, 2017

There looks to be a lot of support for this and it has been almost another month - @esoeylemez any progress on a PR to implement this?

@esoeylemez
Copy link

@esoeylemez esoeylemez commented May 7, 2017

Sorry for the delay. Working on it right now.

@grahamc
Copy link
Member

@grahamc grahamc commented May 7, 2017

As of now, the vote is 13 thumbs-ups and 0 thumbs-downs.

@grahamc
Copy link
Member

@grahamc grahamc commented May 7, 2017

^ I added this comment since votes can be changed later :)

@zimbatm zimbatm merged commit 690b7ca into NixOS:master May 14, 2017
Mic92 added a commit to Mic92/nix-1 that referenced this pull request Jul 30, 2017
Relevant RFC: NixOS/rfcs#4

$ ag -l | xargs sed -i -e "/\"/s/’/'/g;/\"/s/‘/'/g"
@Mic92
Copy link
Contributor

@Mic92 Mic92 commented Jul 31, 2017

This have been now implemented in NixOS/nix#1495

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

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.