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

zig fmt: vertically align adjacent struct init fields #2196

Closed
hryx opened this Issue Apr 5, 2019 · 16 comments

Comments

Projects
None yet
9 participants
@hryx
Copy link
Contributor

hryx commented Apr 5, 2019

Proposal: vertically align the expressions (RHS) of fields in a struct initialization, starting at the = sign. This should only be applied to adjacent lines; empty lines and comments end a group of aligned fields.

This is the policy in go fmt and I've found it significantly helps the readability of certain structs. I can tackle this before 0.4.0 but wanted to get approval first.

Before:

pub const FailingAllocator = struct {
    // field decls...

    pub fn init(allocator: *mem.Allocator, fail_index: usize) FailingAllocator {
        return FailingAllocator{
            .internal_allocator = allocator,
            // interrupted by comment or empty line
            .fail_index = fail_index,
            .index = 0,
            .allocated_bytes = 0,
            .freed_bytes = 0,
            .deallocations = 0,
            .allocator = mem.Allocator{
                .reallocFn = realloc,
                .shrinkFn = shrink,
            },
        };
    }
}

After:

pub const FailingAllocator = struct {
    // field decls...

    pub fn init(allocator: *mem.Allocator, fail_index: usize) FailingAllocator {
        return FailingAllocator{
            .internal_allocator = allocator,
            // interrupted by comment or empty line
            .fail_index      = fail_index,
            .index           = 0,
            .allocated_bytes = 0,
            .freed_bytes     = 0,
            .deallocations   = 0,
            .allocator       = mem.Allocator{
                .reallocFn = realloc,
                .shrinkFn  = shrink,
            },
        };
    }
}

See also: #1793 , #2195

@andrewrk

This comment has been minimized.

Copy link
Member

andrewrk commented Apr 5, 2019

Thanks for the detailed proposal.

The one downside to this that I want to consider is that it is hostile to diffs. With this proposal, a change to a single line turns into a multi-line diff in source control.

I see that you addressed this concern in #1793 (comment). I'd like to let this proposal bake for a little bit and gather some community feedback before making a decision.

@andrewrk andrewrk added this to the 0.5.0 milestone Apr 5, 2019

@andrewrk andrewrk added the proposal label Apr 5, 2019

@tiehuis

This comment has been minimized.

Copy link
Member

tiehuis commented Apr 5, 2019

I personally kind of like this alignment (such as in Go). There is an argument that the diff-noise is the wrong thing to optimize for, preferring instead code readability, so I wouldn't consider that a huge downside.

I would argue that code readability is the main priority, and if it is indeed considered more readable, then it should be accepted.

@thejoshwolfe

This comment has been minimized.

Copy link
Member

thejoshwolfe commented Apr 5, 2019

I concede that the alignment looks nice. Whether it's easier to read is subjective, but I might actually agree that the alignment makes it easier to read (as long as there aren't any really long names ruining it). However, I overall still think we should not do this.

In addition to diff noise, let me throw in another argument against vertical alignment:

  1. You can no longer reliably search for .name = (in your editor or grep etc.) to find assignments and initializations. (This is only marginally useful, but it's something.)

This isn't a big deal, neither is diff noise, and neither is the increase in readability. It's not like this affects the semantics of the program, so weak arguments are all we have to work with on all sides.

So to continue throwing out weak arguments, here's one more

  1. The "rows" of the table-like formatting are totally different from each other, so it doesn't make sense to align the columns.

With array literals in #1793 , each row has the same type, so it makes sense to align the columns. But with struct fields, each row is different. The only thing similar about each row is the syntactic structure, .name = ....

The field names usually have different lengths (which is the motivation for this proposal) because they are describing fundamentally different things. Contrast with arrays literals where you're aligning integers with other integers that have a different number of digits (see the example in #1793 ). When the type is the same in each row, the "meaning" is (usually) the same in each row as well. But differences in field names correspond (to an extent) to different meanings, so aligning them visually doesn't have as much value.

Another way to argue this philosophical objection is to point out how radically different the value expressions can be. In the above example, we've got a scalar 0 followed by a 4-line struct initialization. It's a bit odd that we're trying to align those when they are so different.

...But that's all a subjective philosophical exercise that might not be meaningful to very many people.

Really my biggest objection is the diff noise. Yes you can mitigate the diff noise by ignoring whitespace differences, but that's a mitigation. When you look at things like the --stat for a git commit, it doesn't ignore whitespace. When you look in the margin of your IDE to see which lines have been edited, it doesn't ignore whitespace.

In general, I don't think diff tools are mature enough to be able to understand the difference between important and unimportant changes (which is pretty astonishing given that diff tools are decades old; is no one innovating diff algorithms? (Is no one innovating version control systems?)).

@thejoshwolfe

This comment has been minimized.

Copy link
Member

thejoshwolfe commented Apr 5, 2019

With this proposal, would this be the alignment:

            .deallocations   = 0,
            .allocator       = mem.Allocator{
                .reallocFn = realloc,
                .shrinkFn  = shrink,
            },
            .freed_bytes     = 0,

or this:

            .deallocations   = 0,
            .allocator       = mem.Allocator{
                .reallocFn = realloc,
                .shrinkFn  = shrink,
            },
            .freed_bytes = 0,
@MasonRemaley

This comment has been minimized.

Copy link
Contributor

MasonRemaley commented Apr 5, 2019

I'm not if sure I agree that this is easier to read:

This style aligns all the values, which undeniably makes it easier to quickly pick them out as a group, but if that was the primary way of to interpreting the data at hand then you would've likely used an array or tuple instead of a struct.

I’d argue that if each field is named, you're more often looking for key value pairs, which this style makes harder to reconcile as there's now much more white space you have to trace a horizontal line through to get from the key back to the value or vice versa.

(Note that this argument doesn’t conflict with the fact that zigfmt does this with wrapped lists--there's no key-value relationship to be obscured there)

@hryx

This comment has been minimized.

Copy link
Contributor Author

hryx commented Apr 5, 2019

Lots of good points @thejoshwolfe .

With this proposal, would this be the alignment:

Great point, I accidentally left that ambiguous. go fmt behaves like the second example in your question above — a lonely closing brace such as that breaks the flow, same way that a comment or empty line does — and that approach looks better to me too.

The search issue would bother me if it weren't for the availability of regular expressions. But I realize that may be a barrier for devs who have not learned to use them yet, and it would still add some friction either way.

Personally, the incongruity of types doesn't bother me philosophically.

@MasonRemaley thanks for the perspective — for me, the proposed style is a bit easier to read, as I find my eyes zig-zagging more without it on denser structs.

@MasonRemaley

This comment has been minimized.

Copy link
Contributor

MasonRemaley commented Apr 5, 2019

(Whoops, didn't see Josh's comment prior to posting, I think we're making a similar argument. I also share his concern about how this affects searching.)

@daurnimator

This comment has been minimized.

Copy link
Contributor

daurnimator commented Apr 6, 2019

I have the same concerns as @thejoshwolfe. My 'house' code style used vertical alignment for a year or so, but eventually the lack of simple grepability + the diff noise was causing more work than the vertical alignment helped.

@altermark

This comment has been minimized.

Copy link

altermark commented Apr 8, 2019

There is a way to indent, align names, types, init values, comments etc. without any diff noise, which slowly gains traction as editors gain support. See Elastic tabstops. I use it daily in Python and Go and find it very useful. (Windows, Notepad++, Elastic Tabstops plugin, if you want to try)

And I also find it unfortunate that the decision to enforce the use of spaces everywhere in Zig kind of blocks me from enjoying the little innovation that is happening in the world of plain text editing.

@thejoshwolfe

This comment has been minimized.

Copy link
Member

thejoshwolfe commented Apr 8, 2019

@altermark as innovative as that is, since it isn't widely supported in editors, it would be much worse than using spaces. The hardtab situation in the editor ecosystem today is clearly not ready for that kind of commitment.

@AndreaOrru

This comment has been minimized.

Copy link
Member

AndreaOrru commented Apr 8, 2019

I typically align all my assignments and structure fields unless the language ecosystem offers an automatic formatting tool that does otherwise. I definitely think it makes the code more readable.

In my opinion, as one of the defining principles of Zig is to be “readers first”, readability should trump convenience in this case and we should opt for alignment by default, regardless of the diff issue.

Just my two cents - I don’t feel strongly either way.

@hryx

This comment has been minimized.

Copy link
Contributor Author

hryx commented Apr 8, 2019

Sounds like the main contention is:

@daurnimator: the lack of simple grepability + the diff noise was causing more work

Here's how I normally deal with these issues, for example in Go:

git diff -w
grep -R 'name *=' .

Would that solution work for the things you've encountered too?

Are there any other issues besides the ones brought up which might affect this proposal?

@vegecode

This comment has been minimized.

Copy link
Contributor

vegecode commented Apr 8, 2019

@thejoshwolfe

This comment has been minimized.

Copy link
Member

thejoshwolfe commented Apr 8, 2019

The "diff noise" point is an argument for readability, but not readability for the code's current state. Diff noise negatively impacts readability for changes to the code, whether that's looking through its history, reviewing pull requests, or checking what changes you've typed since starting a feature. Code diffs are a big part of software engineering.

Again, I wish diff technology had innovated at all in the last 20 or so years so this would not be an issue, but that's out of Zig's control.

@daurnimator

This comment has been minimized.

Copy link
Contributor

daurnimator commented Apr 9, 2019

Would that solution work for the things you've encountered too?

No. I often use tools where regex based searches are either not possible or a chore:

  • 'select next occurence' in an editor e.g. Ctrl+D in sublimetext
  • ag -Q 'foo = x * when I'm searching for e.g. an assignment to foo that starts with multiplying by x. I use plaintext mode here as my searches often include characters that are special in regexes.

Note that this is functionality used with reading and reviewing code, and hence I consider vertical alignment a barrier to understanding code.

git diff -w

This hides indentation changes that I consider important when reviewing code. There is no diff argument to show start-of-line whitespace, but not within-line whitespace.

@andrewrk

This comment has been minimized.

Copy link
Member

andrewrk commented Apr 11, 2019

The community seems pretty split on this. I think we've heard reasonable arguments on both sides. I'm personally ambivalent. I was hoping there would be a clear consensus so I didn't have to make a decision, but it seems that I will have to make one after all.

I think that since there's not a clear winner either way, my vote will be to not have this feature. My tiebreaker reasoning is that not doing it is simpler.

One thing I hope that we can all agree on is that there will be no configurability of zig fmt. In the words of the Go community, "Gofmt's style is no one's favorite, yet gofmt is everyone's favorite."

@andrewrk andrewrk closed this Apr 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.