Crystal: Formatter: mixed visibility constants in same segment

Created on 4 Mar 2017 · 11Comments · Source: crystal-lang/crystal

For example:

private Foo = 1
A = 2

How is this file formatted?

I consider 2 behaviors:

```
private Foo = 1
        A   = 2
```
```
private Foo = 1
A           = 2
```

The 1 is pretty good, but this implementation will be heavy, I think. The 2 is not the best, but we can implement it easily.

Which do you like? Or do you have any idea?

feature discussion topicformatter

Source

MakeNowJust

Most helpful comment

I would keep the first one, for me the 2 should not be in the same segment, and maybe the formatter should even separate them.

The visibility is different, the formatting should clearly show that:

private Foo = 1
protected Bar = 4
protected FooBar = 5
Fizz = 10

For me should be formatted as:

private Foo = 1

protected Bar    = 4
protected FooBar = 5

Fizz = 10

bew on 4 Mar 2017

👍6

All 11 comments

I would keep the first one, for me the 2 should not be in the same segment, and maybe the formatter should even separate them.

The visibility is different, the formatting should clearly show that:

private Foo = 1
protected Bar = 4
protected FooBar = 5
Fizz = 10

For me should be formatted as:

private Foo = 1

protected Bar    = 4
protected FooBar = 5

Fizz = 10

bew on 4 Mar 2017

👍6

I wish constants weren't aligned on their assignment. We'd have a nicer-to-read-patch-diff:

  Short = 1
- VeryLongConstant = 2
+ Renamed = 2

Instead of:

- Short            = 1
- VeryLongConstant = 2
+ Short   = 1
+ Renamed = 2

Then @bew's example would lead to no change by the formatter:

private Foo = 1
protected Bar = 4
protected FooBar = 5
Fizz = 10

ysbaddaden on 8 Mar 2017

😕2 👍2

@ysbaddaden unless we switch to elastic-tabstops I think those extra diff lines are things we will need to ~deal with~ live with. And the editors haven't catch up yet to use elastic-tabstops, I don't see that changing any time soon.

bcardiff on 8 Mar 2017

I have more general question.

Should the following segment be separated?

private property foo : String
private property bar : Int32
protected property fizz : String
protected property buzz : String

↓

private property foo : String
private property bar : Int32

protected property fizz : String
protected property buzz : String

In other words, should we separate segments by visibility modifiers?

MakeNowJust on 8 Mar 2017

@MakeNowJust I don't think so. I would aim for aligning constants only.

Otherwise, maybe:

assignments in properties for default values should be aligned also.

property foo = false
property longer_foo = false

segments with properties and plain ivars should be separated.

property foo
@bar = 42
property baz

currently we do nothing on those cases and I sometimes mix them (might not be the best thing to do, but is how my code sometimes evolve, putting related props/ivars together).

constants, since they are read only they don't "suffer" that use case.

bcardiff on 8 Mar 2017

I have a strong preference for the first suggestion in the @MakeNowJust 's message over all the others for 2 reasons:

we can easily read the types, the names and the values with no effort because not only you can read horizontally but also vertically. Each column has a meaning so you can easily review the consistency of the code.
Since each column has a meaning, it's an improvement for security because you can compare lines more easily. The fact that you can read vertically makes debugging way faster for the eye. It almost appears more like an image for the brain than text that you must read line by line.