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

cmd/gofmt: inconsistent formatting of struct with comments #29784

Open
DylanMeeus opened this Issue Jan 17, 2019 · 6 comments

Comments

Projects
None yet
4 participants
@DylanMeeus
Copy link

DylanMeeus commented Jan 17, 2019

What version of Go are you using (go version)?

$ go version 
go1.11.4 windows/amd64

Does this issue reproduce with the latest release?

yes

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
set GOARCH=amd64                                                                                                                                                                                                                                                               
set GOBIN=                                                                                                                                                                                                                                                                      
set GOCACHE=C:\Users\dmeeus1\AppData\Local\go-build                                                                                                                                                                                                                             
set GOEXE=.exe                                                                                                                                                                                                                                                                  
set GOFLAGS=                                                                                                                                                                                                                                                                    
set GOHOSTARCH=amd64                                                                                                                                                                                                                                                            
set GOHOSTOS=windows                                                                                                                                                                                                                                                            
set GOOS=windows                                                                                                                                                                                                                                                                
set GOPATH=C:\Development\Go\src                                                                                                                                                                                                                                                
set GOPROXY=                                                                                                                                                                                                                                                                    
set GORACE=                                                                                                                                                                                                                                                                     
set GOROOT=C:\Go                                                                                                                                                                                                                                                                
set GOTMPDIR=                                                                                                                                                                                                                                                                   
set GOTOOLDIR=C:\Go\pkg\tool\windows_amd64                                                                                                                                                                                                                                      
set GCCGO=gccgo                                                                                                                                                                                                                                                                 
set CC=gcc                                                                                                                                                                                                                                                                      
set CXX=g++                                                                                                                                                                                                                                                                     
set CGO_ENABLED=1                                                                                                                                                                                                                                                               
set GOMOD=C:\Development\Go\moapr\golang\nexuz\moapr\go.mod                                                                                                                                                                                                                     
set CGO_CFLAGS=-g -O2                                                                                                                                                                                                                                                           
set CGO_CPPFLAGS=                                                                                                                                                                                                                                                               
set CGO_CXXFLAGS=-g -O2                                                                                                                                                                                                                                                         
set CGO_FFLAGS=-g -O2                                                                                                                                                                                                                                                           
set CGO_LDFLAGS=-g -O2                                                                                                                                                                                                                                                          
set PKG_CONFIG=pkg-config                                                                                                                                                                                                                                                       
set GOGCCFLAGS=-m64 -mthreads -fmessage-length=0 -fdebug-prefix-map=C:\Users\dmeeus1\AppData\Local\Temp\go-build162758338=/tmp/go-build -gno-record-gcc-switches

What did you do?

I created a struct that has some comments in it. For example:

type T struct {
   a string
   aMuchLongerName string
   // a comment.
   c int
   delta int

Next I hit format. And it formats each comment section as if they are of different blocks.

Just click on format here:
https://play.golang.org/p/wFOqUymxaBY

What did you expect to see?

That all the types would be aligned throughout the entire struct.

What did you see instead?

Alignment in blocks divided by comments.

PS: Maybe this is intentionally so. I couldn't find a related issue with a quick search.
Thanks for taking a look!

~Dylan

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Jan 17, 2019

This is indeed working as intended, although gofmt's behavior is not documented in detail. See #26352 (comment), for example. Perhaps the documentation somewhere could be improved, such as https://golang.org/cmd/gofmt/.

/cc @griesemer

@DylanMeeus

This comment has been minimized.

Copy link
Author

DylanMeeus commented Jan 17, 2019

Thank you :)

Just to give my humble opinion.. it makes the code less readable. See for example: https://play.golang.org/p/T9lxMNkITmz

But I can see the reasoning behind breaking it in "logical blocks" and then formatting those. Hence why I thought it might have been by design ;-)

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Jan 17, 2019

Indeed, not being able to break down the formatting into blocks would be much worse than what we currently have. One could argue that only empty lines should break alignment, and not comments alone, but I wonder if that could be changed at this point. I'll leave that to Robert.

@agnivade

This comment has been minimized.

Copy link
Member

agnivade commented Jan 17, 2019

Sometimes in my code, when I have large structs, I comment in a separate line to group fields, which creates a new "logical block" of fields.

@DylanMeeus - If you just remove the empty //, I think it is pretty readable and clear.

type T struct {
	a               string
	aMuchLongerName string

	// comment
	c     int
	delta int

	// and another!
	b           int32
	theseAre    string
	LongerAgain string
}

@agnivade agnivade changed the title gofmt inconsistent formatting of struct with comments cmd/gofmt: inconsistent formatting of struct with comments Jan 17, 2019

@mvdan

This comment has been minimized.

Copy link
Member

mvdan commented Jan 17, 2019

@agnivade I think what we could potentially do, which is what I tried to explain above, is keep the alignment in the scenario where the lines in between fields aren't truly empty:

type Foo struct {
        // F1 is ...
        F1   string
        // FF2 is ...
        FF2  string
        // FFF3 is ...
        FFF3 string
}

Not saying it's definitely a good idea, but it doesn't look unreasonable to me.

@DylanMeeus

This comment has been minimized.

Copy link
Author

DylanMeeus commented Jan 18, 2019

For what it's worth, I think that's a good idea @mvdan.

How I stumbled on this bug was that I (temporarily) wanted to remove something from a struct, though all fields did logically belong together. But then it grouped the imports because of the commented-out field :-)

Sometimes you'd want to add comments without declaring a new 'logical block', so it makes sense to keep them together. And other times you'd want to split them up.

What @agnivade suggested is a good work-around and keeps the readability of the struct. Though it'd be nice to also have the option to keep all fields as one block, even with comments.

@bradfitz bradfitz added this to the Go1.13 milestone Jan 18, 2019

@DylanMeeus DylanMeeus closed this Jan 18, 2019

@DylanMeeus DylanMeeus reopened this Jan 18, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment