Crystal: [Formatter] Right-aligned number literals should anchor on units column

[straight-shoota]

When the formatter aligns number literals (for example in consecutive constant declarations), they are pushed all the way to the right. This primitive method is only effective in aligning columns when all numbers have the same type, precision and format.

When they are different, the formatter applies this for example:

FOO =  310_u32
BAR = 18101914
BAZ = 1_908.15

This doesn't help at all to grasp and compare the magnitude of the numbers, especially if more digits are included.

A proper alignment would be:

FOO =        310_u32
BAR = 18_101_914
BAZ =      1_908.15

It is debatable if the formatter should insert underscore. But even without chaning the format of the literal, it would be better readable:

FOO =      310_u32
BAR = 18101914
BAZ =    1_908.15

[jwoertink]

I personally find the first example to be the most readable. The last example looks like it was done by mistake to me.

[Sija]

I'd go for left alignment:

FOO = 310_u32
BAR = 18101914
BAZ = 1_908.15

[RX14]

I like the right-aligning. But if we keep it it should align by the actual numbers, ignoring types and decimals. This aligning seems to be a controversial choice though.

[larubujo]

id go for no alignment at all:

FOO = 1
BARBAZ = 2

why some assigns are aligned, others not? why constants special? plus no aligning, no one complains. align in some way, some will complain.

[RX14]

@larubujo i'd certainly complain if there wasn't aligning.

[larubujo]

why complain there but not with regular vars:

a = 1
bc = 2

?

or with instance var, class var, etc

[straight-shoota]

@larubujo Important constant numbers are typically not assigned directly to variables but defined as constants. And those are the ones we like to have aligned.

Another issue with this: Alignment is only coordinated between immediately consecutive lines. That makes sense so you can structure constants in groups and dont have all of them aligned together.
However, it makes it impossible to document these constants. This is how the formatter puts things:

FOO = 18101914
# BAR is the answer
BAR = 42

FOO = 18101914
BAR =       42

If the alignment doesn't work with documentation comments in between, it's not really useful.

[larubujo]

right, i find the second harder to read. the more you spread left and right, harder to read. what is easier?

A = 1
B = 2
C = 123456789_123456789_i64

or

A =                       1
B =                       2
C = 123456789_123456789_i64

my eyes go left right left right left right to understand that. maybe good exercise for eyes, though

[straight-shoota]

A = 23_456_789_123_456_789
B = 123_456_789_123_456_789

vs

A =  23_456_789_123_456_789
B = 123_456_789_123_456_789

You can't seriously prefer the first one in any way.

[ghost]

A =                       1
B =                       2
C = 123456789_123456789_i64

most constants that are grouped will have similar length or will be separated with a linebreak though and even if this might sometimes not be the case this is a rather extreme example that will work just fine 99% of the time

it makes sense to align integers right based on what the value of each digit is

but does it make sense to align floats right instead of at their separator?

# currently
F  =  0.56_f32
F2 = 0.561_f32

vs

# (I'd prefer)
F  =  0.56_f32
F2 =  0.561_f32

[straight-shoota]

@monouser7dig That's what this PR is about: aligning to the units column, so the decimal points would also be in the same column.

[ysbaddaden]

There should never be any alignment, neither in const/var names nor in values. It's most often than not completely weird, especially with very different lengths. That would since issues, because there would be none :D

I know, everybody wants to align things. I must be the weird one.

[RX14]

How about this compromise: always preserve alignment for declarations of any type?

[straight-shoota]

That's certainly better than the current half-hearted solution which causes new issues. If the formatter doesn't enforce anything, the developer is at least able to do what fits best.

However, we'd need to find a way to distinguish intentional whitespace from unintentional... For example in this example, it would be great if the second space characters after = would be removed.

foo =  1
bar =  2

And just to be clear: Alignment of = should be kept as is, I don't think there are any issues with that.

[straight-shoota]

That's certainly better than the current half-hearted solution which causes new issues. If the formatter doesn't enforce anything, the developer is at least able to do what fits best.

However, we'd need to find a way to distinguish intentional whitespace from unintentional... For example in this example, it would be great if the second space characters after = would be removed.

FOO =  1
BAR =  2

And just to be clear: Alignment of = should be kept as is, I don't think there are any issues with that.

[jeff-hykin]

I definitely support RX14's solution, leaving it to the dev and/or situation

A = 1
B = 2
C = 123456789_123456789_i64

^that I believe is easier to read left-aligned

A =  23_456_789_123_456_789
B = 123_456_789_123_456_789

but ^that Is certainly easier to read right-aligned

@straight-shoota
I think by default assuming it is intentional would be the least annoying/most useful behavior. Then for special cases like the

FOO =  1
BAR =  2

A rule can be made, e.g. in this case, remove 1 whitespace from every relevant line till at least one line only has a gap of 1 whitespace.

FOO =  1 #>>> FOO = 1  
BAR =  2 #>>> BAR = 2

FOO =      1   #>>> FOO =    1  
BAR =   2000   #>>> BAR = 2000

[RX14]

It's a simple rule, if there's n > 1 whitespace between every = and the value in every declaration in the "block", remove n - 1 whitespace from every line after the =.

[straight-shoota]

A critical question is the definition of a block. Does it only include directly consecutive Const declarations or are comments allowed? What about empty lines? Maybe I want to structure and document constant values but stillkeep them aligned.

[miketheman]

codetriage

This issue has been open since Dec 2017, and there's clearly no consensus as to the styles being considered here - measured by the 👍/ 👎 reactions, as well as the discussions.

The current behavior, given a file like:

FOO = 1
BAR = 2

Adding BAZ = 200 to the file, the behavior of crystal tool format will produce a diff like:

-FOO = 1
-BAR = 2
+FOO =   1
+BAR =   2
+BAZ = 200

I personally dislike this, as well as any other formatting that changes the block with spacing only, as it increases the difficulty of reading a diff for an addition-only change.
Is this surmountable? Certainly - adding ?w=1 to a GitHub diff URL, or using git diff --ignore-space-change ... - these are possible, but not awesome.

Would there be some supporting empirical research on alignment in source code that could help decide this? Or do we leave it as-is for now.

[straight-shoota]

Hm, I think we could probably agree on removing alignment formatting and only remove excess whitespace as @RX14 proposed.

The only issue is how this should be scoped in order to allow manual alignment of consecutive constants with comments/new lines in between.

[j8r]

I also agree to not change the formatting of variable declaration.

A further improvement may be to detect the indent – right, left or else center – based on the first two declarations, and keep this alignement for the following declarations in a row.