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

Tracking issue for access to Formatter alignment #27726

Closed
alexcrichton opened this issue Aug 12, 2015 · 20 comments
Closed

Tracking issue for access to Formatter alignment #27726

alexcrichton opened this issue Aug 12, 2015 · 20 comments
Assignees
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

alexcrichton commented Aug 12, 2015

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

@alexcrichton alexcrichton added T-libs-api Relevant to the library API team, which will review and decide on the PR/issue. B-unstable Blocker: Implemented in the nightly compiler and unstable. labels Aug 12, 2015
@nagisa
Copy link
Member

nagisa commented Aug 13, 2015

Should the formatting flags be boolean accessors?

Yes, please.

Should setters as well as getters be exposed?

Why? Makes little sense.

@alexcrichton
Copy link
Member Author

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
Copy link
Member

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
Copy link
Member Author

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
Copy link
Member Author

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 alexcrichton added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed I-nominated labels Sep 24, 2015
@alexcrichton
Copy link
Member Author

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
Copy link
Contributor

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
Copy link
Member Author

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
Copy link
Member Author

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

alexcrichton added a commit to alexcrichton/rust that referenced this issue Oct 25, 2015
This commit stabilizes and deprecates library APIs whose FCP has closed in the
last cycle, specifically:

Stabilized APIs:

* `fs::canonicalize`
* `Path::{metadata, symlink_metadata, canonicalize, read_link, read_dir, exists,
   is_file, is_dir}` - all moved to inherent methods from the `PathExt` trait.
* `Formatter::fill`
* `Formatter::width`
* `Formatter::precision`
* `Formatter::sign_plus`
* `Formatter::sign_minus`
* `Formatter::alternate`
* `Formatter::sign_aware_zero_pad`
* `string::ParseError`
* `Utf8Error::valid_up_to`
* `Iterator::{cmp, partial_cmp, eq, ne, lt, le, gt, ge}`
* `<[T]>::split_{first,last}{,_mut}`
* `Condvar::wait_timeout` - note that `wait_timeout_ms` is not yet deprecated
  but will be once 1.5 is released.
* `str::{R,}MatchIndices`
* `str::{r,}match_indices`
* `char::from_u32_unchecked`
* `VecDeque::insert`
* `VecDeque::shrink_to_fit`
* `VecDeque::as_slices`
* `VecDeque::as_mut_slices`
* `VecDeque::swap_remove_front` - (renamed from `swap_front_remove`)
* `VecDeque::swap_remove_back` - (renamed from `swap_back_remove`)
* `Vec::resize`
* `str::slice_mut_unchecked`
* `FileTypeExt`
* `FileTypeExt::{is_block_device, is_char_device, is_fifo, is_socket}`
* `BinaryHeap::from` - `from_vec` deprecated in favor of this
* `BinaryHeap::into_vec` - plus a `Into` impl
* `BinaryHeap::into_sorted_vec`

Deprecated APIs

* `slice::ref_slice`
* `slice::mut_ref_slice`
* `iter::{range_inclusive, RangeInclusive}`
* `std::dynamic_lib`

Closes rust-lang#27706
Closes rust-lang#27725
cc rust-lang#27726 (align not stabilized yet)
Closes rust-lang#27734
Closes rust-lang#27737
Closes rust-lang#27742
Closes rust-lang#27743
Closes rust-lang#27772
Closes rust-lang#27774
Closes rust-lang#27777
Closes rust-lang#27781
cc rust-lang#27788 (a few remaining methods though)
Closes rust-lang#27790
Closes rust-lang#27793
Closes rust-lang#27796
Closes rust-lang#27810
cc rust-lang#28147 (not all parts stabilized)
bors added a commit that referenced this issue Oct 25, 2015
This commit stabilizes and deprecates library APIs whose FCP has closed in the
last cycle, specifically:

Stabilized APIs:

* `fs::canonicalize`
* `Path::{metadata, symlink_metadata, canonicalize, read_link, read_dir, exists,
   is_file, is_dir}` - all moved to inherent methods from the `PathExt` trait.
* `Formatter::fill`
* `Formatter::width`
* `Formatter::precision`
* `Formatter::sign_plus`
* `Formatter::sign_minus`
* `Formatter::alternate`
* `Formatter::sign_aware_zero_pad`
* `string::ParseError`
* `Utf8Error::valid_up_to`
* `Iterator::{cmp, partial_cmp, eq, ne, lt, le, gt, ge}`
* `<[T]>::split_{first,last}{,_mut}`
* `Condvar::wait_timeout` - note that `wait_timeout_ms` is not yet deprecated
  but will be once 1.5 is released.
* `str::{R,}MatchIndices`
* `str::{r,}match_indices`
* `char::from_u32_unchecked`
* `VecDeque::insert`
* `VecDeque::shrink_to_fit`
* `VecDeque::as_slices`
* `VecDeque::as_mut_slices`
* `VecDeque::swap_remove_front` - (renamed from `swap_front_remove`)
* `VecDeque::swap_remove_back` - (renamed from `swap_back_remove`)
* `Vec::resize`
* `str::slice_mut_unchecked`
* `FileTypeExt`
* `FileTypeExt::{is_block_device, is_char_device, is_fifo, is_socket}`
* `BinaryHeap::from` - `from_vec` deprecated in favor of this
* `BinaryHeap::into_vec` - plus a `Into` impl
* `BinaryHeap::into_sorted_vec`

Deprecated APIs

* `slice::ref_slice`
* `slice::mut_ref_slice`
* `iter::{range_inclusive, RangeInclusive}`
* `std::dynamic_lib`

Closes #27706
Closes #27725
cc #27726 (align not stabilized yet)
Closes #27734
Closes #27737
Closes #27742
Closes #27743
Closes #27772
Closes #27774
Closes #27777
Closes #27781
cc #27788 (a few remaining methods though)
Closes #27790
Closes #27793
Closes #27796
Closes #27810
cc #28147 (not all parts stabilized)
@mbrubeck
Copy link
Contributor

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

@mbrubeck mbrubeck reopened this Mar 31, 2017
@Mark-Simulacrum Mark-Simulacrum added the C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. label Jul 22, 2017
@partim
Copy link
Contributor

partim commented Jan 1, 2018

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
Copy link
Member

@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-}

@Mark-Simulacrum Mark-Simulacrum removed this from the 1.5 milestone Jan 15, 2018
@Mark-Simulacrum Mark-Simulacrum changed the title Tracking issue for access to Formatter flags/options Tracking issue for access to Formatter fmt_align Jan 15, 2018
@Mark-Simulacrum Mark-Simulacrum changed the title Tracking issue for access to Formatter fmt_align Tracking issue for access to Formatter alignment Jan 15, 2018
@sfackler
Copy link
Member

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

@rfcbot fcp merge

@rfcbot rfcbot added the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jan 23, 2018
@rfcbot
Copy link

rfcbot commented Jan 23, 2018

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 rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Feb 14, 2018
@rfcbot
Copy link

rfcbot commented Feb 14, 2018

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

1 similar comment
@rfcbot
Copy link

rfcbot commented Feb 14, 2018

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

@rfcbot rfcbot added final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. and removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Feb 14, 2018
@rfcbot
Copy link

rfcbot commented Feb 24, 2018

The final comment period is now complete.

tmccombs added a commit to tmccombs/rust that referenced this issue Apr 30, 2018
@GuillaumeGomez
Copy link
Member

Shouldn't this get merged then?

@SimonSapin
Copy link
Contributor

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

@Centril Centril added disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels May 24, 2018
@GuillaumeGomez GuillaumeGomez self-assigned this May 25, 2018
bors added a commit that referenced this issue May 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B-unstable Blocker: Implemented in the nightly compiler and unstable. C-tracking-issue Category: A tracking issue for an RFC or an unstable feature. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. T-libs-api Relevant to the library API team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests