Interested in implementing a change to go fmt struct tags alignment

[Matt Cudmore]

I am interested in refining the code that aligns the various parts of struct fields.

Currently the alignments are wonky and difficult to read if a contiguous list of fields include a mix of identified and anonymous fields, tags, and comments.

For example (ugliest example I could make): Actual.png (attached)

Normally I define and group fields more carefully and the formatting is nice, but there are cases where it is natural to include anonymous, tagged, and commented fields contiguous with others, and situations where I think contiguous alignment per part (names/embedded, types for named field, tags, and then comments) would be better.

Here's what I have in mind: Concept.png (attached)

To do this would require something akin to computing the positions of the types for named fields in a contiguous section (until a line that doesn't define a field), aligning those to the rightmost, and then repeating for tags, and then for comments. If there is only one type, tag, or comment in a contiguous section, then that part can float as far left as possible, as it already does. At minimum this will require changes to printer.fieldList in src/pkg/go/printer/nodes.go

There are a few ambiguous cases that would have to be identified precisely. For example, currently the formatter aligns the first parts, the second parts, and so on, resetting alignment where an intermediate line doesn't have the same number of parts: Ambig.png (attached)

The changes I would like to investigate would align the parts per kind, rather than per number, and (possibly, to be decided) would align all the parts of the same kind within a contiguous list of fields until a blank line or comment rather than resetting at a line not including the same kind of part.

The only downside that comes to mind is that if this kind of change were merged, the next release would mean reformatted code all across the board.

At this point I am looking for feedback on whether these refinements would be welcomed.

- Matt

[minux]

From your pictures, this seems like an enhancement. But as I'm certainly not the one who

can decide such issue, I suggest you to file an issue for this, and include links to play.golang.org

link for all your example code snippets, and also the expected version so that people can easily

experiment themselves.

[Oleku Konko]

Can Switch indentation also bee looked at ... its minor but make the code more readable example :

Current go fmt result  

 Proposed 

[Aram Hăvărneanu]

> Can Switch indentation also bee looked at ... its minor but make the code more readable example :

No way.

--
Aram Hăvărneanu

[chris dollin]

On 23 January 2014 13:41, Oleku Konko <oleku...@gmail.com> wrote:

Can Switch indentation also bee looked at ... its minor but make the code more readable example :

I seem to remember that the feature you're disliking -- unindented cases -- is

a deliberate choice of the Go team, so the odds of  it changing a low.

(I don't see a significant readability loss with the current layout. Or even a loss.)

Chris

--
Chris "allusive" Dollin

[Matt Cudmore]

Thanks for the tip. I opened the issue at:

https://code.google.com/p/go/issues/detail?id=7199

There are a few other open issues I noticed related to the go fmt output:

https://code.google.com/p/go/issues/detail?id=4504

https://code.google.com/p/go/issues/detail?id=4808

https://code.google.com/p/go/issues/detail?id=7004

Rather than patching I wonder if someone should take on the work of rewriting the go/printer package. Currently the code is very difficult to digest and doesn't appear to be using a very elegant model. The related packages go/ast and go/token may be fine as they are.

[Robert Griesemer]

Hi there (I'm the author of gofmt and all the go/* libraries it depends on). Thanks for filing the issues.

gofmt is notoriously difficult to adjust, some of it is due to its design (which evolved over time), and some of it is due to the inherent difficulty of formatting code in a nice way.

I would agree that gofmt could use a rewrite. That said, the real problem is actually not gofmt, but go/ast, or more precisely, the way comments are represented (rather "not represented") in the go/ast. gofmt is all about dealing with comments, the rest is comparatively trivial. (I've been thinking about a go/ast rewrite - think next-gen ast, but it's not quite ready for a serious discussion yet).

Regarding the suggested changes: Most of the pending gofmt issues can be fixed in the existing source base, and I am planning to pay attention to that soon, before the 1.3 freeze. I'd prefer to do these changes myself; but if you cannot wait you could have a look at some of the variable/constant declaration layout code which tries to be a bit smarter about the arrangement. I think something a long the same lines can be done for struct fields.

I'd not recommend a wholesale gofmt rewrite at this point. It's a bigger project than one might anticipate.

Thanks.

- gri

PS: The alignment of switch cases is not going to change. That was a deliberate decision made very early on.

--
You received this message because you are subscribed to the Google Groups "golang-nuts" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-nuts...@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

[Carlos Castillo]

I have made a proof of concept CL here: https://codereview.appspot.com/56360043

It currently breaks two tests in go/printer, and has the old code commented out, but it's a pretty small change otherwise.

[Carlos Castillo]

Aside from what I put in the notes section, I also won't fix/mail it yet because of what @gri said about wanting to do it himself.

[Matt Cudmore]

Hey, thanks for the reply.

Glad to know these concerns are in good hands. I'll wait.

- Matt

[John Fries]

Oleku, you are not alone. I too was initially bothered by gofmt's alignment of switch and case statements. In many ways, it's natural for there to be indentation for the case statements. However, indentation comes with a cost as well, and I've come to believe that the cost outweighs the benefit in this case.

--

[bjbo...@gmail.com]

Hi, is your code still at that codereview.appspot.com link?  I took a look, and there seem to be four "patch set"s, two of which have some changes to nodes.go, but the changes look too minimal to make a dent in this problem.

Thanks,

Bryan