rust-lang / rust

Empowering everyone to build reliable and efficient software.
https://www.rust-lang.org
Other
98.2k stars 12.7k forks source link

Tracking issue for access to Formatter alignment #27726

Closed alexcrichton closed 6 years ago

alexcrichton commented 9 years ago

Tracking issue for the fmt_flags_align feature, which is fmt::Formatter::align and the associated enum.

nagisa commented 9 years ago

Should the formatting flags be boolean accessors?

Yes, please.

Should setters as well as getters be exposed?

Why? Makes little sense.

alexcrichton commented 9 years ago

I disagree that it makes little sense to expose getters, it's possible to envision a situation where one formatting is composed of many others and you perhaps want some to be formatted to various widths or perhaps enable various flags (e.g. the "alternate" mode). It's certainly niche but it's somewhat necessary for completeness.

sfackler commented 9 years ago

align, width, and precision all seem straightforward and can be stabilized as-is.

I would also very much like to add and stabilize boolean accessors for the various parts of flags: sign_plus, sign_minus, alternate, and zero_pad (we could call the last one sign_aware_zero_pad but that seems like a genuinely excessive name). It's pretty unfortunate we stabilized the flags accessor in the first place IMO, but oh well.

I'm a little less sure about fill, though. What char is returned if no fill character was specified? Why doesn't it return an Option like the others? Is char sufficient? Should it be a full grapheme instead?

alexcrichton commented 9 years ago

I'd be fine just deprecating the flags method (it's basically useless without constants anyway) and adding boolean accessors, I agree that it better matches design in today's Rust.

I'd also be fine making fill return an Option<char> and just specifying that it's always one unicode character (it's how the format string is parsed). Perhaps in theory a grapheme could be specified but that seems like something for an even fancier formatting system!

alexcrichton commented 9 years ago

This issue is now entering its cycle-long FCP for stabilization in 1.5

The accessors being added in #28615 will also be considered for stabilization.

alexcrichton commented 9 years ago

Ah, and @SimonSapin, may be good to get your opinion on the grapheme-vs-char situation here. Currently whenever something is formatted you have the ability to specify a "fill" character which for when padding is applied (normally this is just an ascii space). As @sfackler points out this fill is currently just a char, but it could in theory be a grapheme in terms of "one thing on the screen". Do you have an opinion either way in that regard? Curious to hear thoughts!

SimonSapin commented 9 years ago

Uh. It’s not just graphemes. The whole width/align/fill thing in std::fmt as currently written is based on some assumptions:

  1. We’re printing to something that (like most terminal emulators) align text on a grid
  2. The number of grid slots occupied by a piece of text is the number of UTF-8 bytes (&str::len).
  3. It’s the number of char code points
  4. Printing a char occupies one slot on the grid

Unfortunately, as often with Unicode, it’s complicated. Not only grapheme clusters can have more than one code point and still only use one slot (with combining code points), but most characters of some Asian languages and most emoji are “full width”: they’re twice the usual width in most monospace fonts. Control characters might or might not be displayed. Or they might be interpreted by the terminal to move the cursor around.

http://eev.ee/blog/2015/09/12/dark-corners-of-unicode/#combining-characters-and-character-width has some more background. Here is an extract, about emoji flags:

So the “length” of a pair of these characters depends both on the display font (which may not support all flags), and on the current geopolitical state of the world. How’s that for depending on global mutable state?


Our best bet for ”what is the width of this string?” is probably https://github.com/unicode-rs/unicode-width

That leaves dealing with fill if it’s double width or a control character. If we want to do it, could fill be an arbitrary string rather than a single char? Or should width(fill) != 1 be rejected?

alexcrichton commented 9 years ago

Hm, those are very good points! I would be mostly tempted to just pare down everything and say it largely deals with ascii only (and maybe simple unicode?). I don't think it'd be too beneficial to start getting full-blown unicode-width support in libcore just to support a use case like this.

Having some verification in the compiler, however, to make sure you're not doing crazy things seems reasonable?

If we want to do it, could fill be an arbitrary string rather than a single char?

In theory, yes.

Or should width(fill) != 1 be rejected?

I'd also be fine with this!

alexcrichton commented 9 years ago

The libs team discussed this during triage today and the decision was to stabilize.

steveklabnik commented 7 years ago

Triage: looks like this was stabilized, closing!

mbrubeck commented 7 years ago

Formatter::align is still unstable, linking to this issue: https://doc.rust-lang.org/nightly/std/fmt/struct.Formatter.html#method.align

partim commented 6 years ago

I am a bit confused about the reason Formatter::align is still unstable. Is it because Alignment is only available from core?

Is there anything I can help with to move this along?

Mark-Simulacrum commented 6 years ago

@rust-lang/libs Nominating for stabilization. I suspect that Alignment needs to be re-exported in std, but that seems like something trivial to solve while stabilizing. The only concern that I see left is that there appear to be 3 separate, but equivalent, definition of the Alignment enum in the various libraries. These should probably be de-duplicated, but again, since only one appears to be public, we can probably do this trivially as well during stabilization.

src/libfmt_macros/lib.rs
86:pub enum Alignment {
87-    /// The value will be aligned to the left.
88-    AlignLeft,
89-    /// The value will be aligned to the right.
90-    AlignRight,
91-    /// The value will be aligned in the center.
92-    AlignCenter,
93-    /// The value will take on a default alignment.
94-    AlignUnknown,
95-}

src/libcore/fmt/mod.rs
31:pub enum Alignment {
32-    /// Indication that contents should be left-aligned.
33-    Left,
34-    /// Indication that contents should be right-aligned.
35-    Right,
36-    /// Indication that contents should be center-aligned.
37-    Center,
38-    /// No alignment was requested.
39-    Unknown,
40-}

src/libcore/fmt/rt/v1.rs
35:pub enum Alignment {
36-    /// Indication that contents should be left-aligned.
37-    Left,
38-    /// Indication that contents should be right-aligned.
39-    Right,
40-    /// Indication that contents should be center-aligned.
41-    Center,
42-    /// No alignment was requested.
43-    Unknown,
44-}
sfackler commented 6 years ago

I think we should remove Alignment::Unknown and return an Option<Alignment> instead, but it seems reasonable to stabilize.

@rfcbot fcp merge

rfcbot commented 6 years ago

Team member @sfackler has proposed to merge this. The next step is review by the rest of the tagged teams:

No concerns currently listed.

Once these reviewers reach consensus, this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

rfcbot commented 6 years ago

:bell: This is now entering its final comment period, as per the review above. :bell:

rfcbot commented 6 years ago

:bell: This is now entering its final comment period, as per the review above. :bell:

rfcbot commented 6 years ago

The final comment period is now complete.

GuillaumeGomez commented 6 years ago

Shouldn't this get merged then?

SimonSapin commented 6 years ago

Yes, with FCP to stabilize completed, the next step is a stabilization PR.