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

Center Align numbers right-ish #883

Merged
merged 1 commit into from
Jan 19, 2022
Merged

Conversation

th1000s
Copy link
Contributor

@th1000s th1000s commented Jan 3, 2022

There is no such thing as "Center Align" with discrete terminal cells. In
some cases a decision has to be made whether to use the left or the right
cell, e.g. when fitting one char into 4 cells: _X__ or __X_.

The format!() center/^ default is left, but when padding numbers these
are now aligned to the right if required. Strings remain left-aligned.


I noticed this while adding line numbers to the blame output.

For my taste the center-right alignment of numbers looks better, see below and let me know what you think.

I think that makes a good default, otherwise extra format specifiers might be added, e.g.
"center left: {num:^<4} center right {num:^>4}"

string: │ 3  │     num: │  3 │
string: │ 4  │     num: │  4 │
string: │ 5  │     num: │  5 │
string: │ 6  │     num: │  6 │
string: │ 7  │     num: │  7 │
string: │ 8  │     num: │  8 │
string: │ 9  │     num: │  9 │
string: │ 10 │     num: │ 10 │
string: │ 11 │     num: │ 11 │
string: │ 12 │     num: │ 12 │
string: │ 13 │     num: │ 13 │
string: │ 94 │     num: │ 94 │
string: │ 95 │     num: │ 95 │
string: │ 96 │     num: │ 96 │
string: │ 97 │     num: │ 97 │
string: │ 98 │     num: │ 98 │
string: │ 99 │     num: │ 99 │
string: │100 │     num: │ 100│
string: │101 │     num: │ 101│
string: │102 │     num: │ 102│
string: │103 │     num: │ 103│
string: │104 │     num: │ 104│
string: │105 │     num: │ 105│
string: │106 │     num: │ 106│
string: │107 │     num: │ 107│
string: │108 │     num: │ 108│
string: │109 │     num: │ 109│
string: │110 │     num: │ 110│
string: │111 │     num: │ 111│
string: │998 │     num: │ 998│
string: │999 │     num: │ 999│
string: │1000│     num: │1000│

src/format.rs Outdated
impl CenterRightNumbers for usize {
fn width_for_center_right(&self) -> usize {
// log10 for integers is only in nightly and this is faster than
// casting to f64 and back.
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe say explicitly that this is 1 + floor(log10(n)) (if I got that right)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. This specific log function was actually my original motivation add benchmarks to delta :)

There is no such thing as "Center Align" with discrete terminal cells. In
some cases a decision has to be made whether to use the left or the right
cell, e.g. when centering one char in 4 cells: "_X__" or "__X_".

The format!() center/^ default is left, but when padding numbers these
are now aligned to the right if required. Strings remain left-aligned.
@th1000s th1000s marked this pull request as ready for review January 18, 2022 22:38
@dandavison
Copy link
Owner

Excellent, I hope the test alterations weren't too painful!

@dandavison dandavison merged commit 3aed51c into dandavison:master Jan 19, 2022
@th1000s
Copy link
Contributor Author

th1000s commented Jan 20, 2022

No, that was mostly sed -i 's,│ 1 │,│ 1 │,g' :)

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

Successfully merging this pull request may close these issues.

None yet

2 participants