-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Use Unicode line drawing characters for spans #28902
Conversation
With some other cleanup at the use sites.
…utput I bet we can find other uses for these sorts of characters in clarifying type errors, etc. This is marked as a "stable" flag for consistency with --color. However we should reconsider both of them as part of rust-lang#19051. For these features to be conveniently available with Cargo, we may want to use environment variables or a config file.
(rust_highfive has picked a reviewer for you, use r? to override) |
(Rather busy right now, might not get time to review, sorry. @alexcrichton, could you have a look through this?) |
Thanks for picking this up @LeoTestard! I think there's definitely an interesting question of defaults here, so it's something I wouldn't mind exploring a bit. I would be a little wary to turn this on by default as I haven't seen much precedent for CLI tools using unicode symbols by default, but on the other hand I feel like if it's turned off by default it may just end up being dead code in the compiler. For example Cargo wouldn't have an easy method of passing this down to the compiler so it'd probably end up happening that the vast majority of compiler invocations don't pass this option. So along those lines, this seems cool, but I'm a little wary about landing this for now. cc @rust-lang/tools, though, other opinions? |
gcc uses unicode (smart quotes) by default in a UTF-8 locale since at least version 4.6.1 (earliest I have handy to test):
|
@alexcrichton I agree. We have several options here. As @wthrowe and other people said, GCC does use Unicode output by default when the locale includes UTF-8. An other option would be an environment variable, as suggested in the previous thread. This way the user can specify that it wants the Unicode output to be enabled without Cargo having to know anything about it. A third possibility is a global configuration file for Cargo in which it is possible to specify this kind of things. Both those options allow not to set Unicode output as the default in the compiler. |
Aha fascinating! That seems like a plausible vector do discover whether this should be turned on by default, and then an environment variable to force it off seems plausible. |
Note that for the Windows console, the console encoding doesn't matter since we use the wide functions anyway. What really matters is the current console font, which we can get the name of via |
Opinion: I'd like to see this, but I also worry about the defaults. I'd prefer to be implicit if we can, rather than use command line options or a config file. If we can be sure the user can handle the unicode lines, we should do it. But we should never do it if we can't. Can we ensure that? Or am I dreaming? In the future, errors for tools will be serialised data structures, rather than text, so there is no issue in that context. |
Travis failure is legit (failed tidy). |
☔ The latest upstream changes (presumably #29285) made this pull request unmergeable. Please resolve the merge conflicts. |
Do you mind if I take this over? |
Closing due to inactivity, but feel free to resubmit with a rebase! |
@sanxiyn I'd still like to work on this PR, but I've got the impression that there is no general consensus about what to do for the options and the defaults. |
@nrc now that we're in the future with proper json errors, should we push this again? |
Yeah, I guess we can. I think what to do by default is the big question here, but it sounds like checking the locale is an acceptable way forward. So, if that works, let's do it! |
See #24387.
This is a port of the first PR (#21406) to the current master branch. I also added a commit to change the characters to use WGL4 instead, as suggested in the comment thread of the original PR, since those are more widely supported.
The proposal that was more or less accepted was to remove the
--color
and--drawing
options and replace them with environment variables and a single--output-plain-ascii
option. The problem is that--color
still exists, and a stable option. I left--drawing
for now, for consistency with--color
but there are maybe better options.This PR is not yet ready to be merged but I wanted to post it to have feedback on this issue. The proposal also included auto-enabling Unicode output when writing to a terminal and LC_CTYPE ends with
.UTF-8
. I'll implement this if there is still agreement that this should be done.cc @Manishearth.
(PS: I did not change the author of the commits since I did not implement the Unicode outputting itself, but the commits from the original PR needed a lot of fixes to build on the current master branch, maybe I should have. Let me know if I should. Doing a separate commit was also possible but generated a huge diff of duplicate modifications.)