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.

Sign up for GitHub

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

Jump to bottom

go/printer: Fprintf uses different settings than go fmt #16963

Closed
kevinburke opened this issue Sep 2, 2016 · 8 comments
Closed

go/printer: Fprintf uses different settings than go fmt #16963

kevinburke opened this issue Sep 2, 2016 · 8 comments
Labels
FrozenDueToAge
Milestone
Go1.8

Comments

@kevinburke
Copy link
Contributor

kevinburke commented Sep 2, 2016 •
edited
Loading

Please answer these questions before submitting your issue. Thanks!

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

1.7

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

All

What did you do?

If I write a Go file with go/printer.Fprintf, structs get printed like this - I've replaced the tab characters with \t so it's clear what is going on.

type User struct{
\tName\t\tstring
\tLongConfig\tuint
}

If you immediately run go fmt ./... on the file, some of the tabs are replaced by spaces:

type User struct{
\tName        string
\tLongConfig  uint
}

go/printer.Fprintf uses the following default printer.Config argument:

return (&Config{Tabwidth: 8}).Fprint(output, fset, node)

However, go fmt uses this config argument:

const (
    tabWidth    = 8
    printerMode = printer.UseSpaces | printer.TabIndent
)

// ...

res, err := format(fileSet, file, sourceAdj, indentAdj, src, printer.Config{Mode: printerMode, Tabwidth: tabWidth})

What did you expect to see?

I expect the two "default" functions to print the file the same way

What did you see instead?

One prints with tabs in the middle and one doesn't. This led to a lot of thrash when running anything automated over the codebase; I noticed this first as part of the investigation for Shyp/bump_version#1.

Far more people run go fmt than write AST parsing/hacking functions, so it seems like the fix, if there is to be one, should be to have printer.Fprintf match the go fmt behavior.
@kevinburke
Copy link
Contributor Author

kevinburke commented Sep 2, 2016 •
edited
Loading

I've signed the CLA and happy to submit a fix, but not sure if this would count as a breaking API change, or is desirable.

@rakyll
Copy link
Contributor

rakyll commented Sep 2, 2016

/cc @griesemer

@bradfitz bradfitz changed the title default go/printer.Fprintf uses different settings than go fmt go/printer: Fprintf uses different settings than go fmt Sep 2, 2016
@quentinmit quentinmit added this to the Go1.8 milestone Sep 6, 2016
@quentinmit
Copy link
Contributor

Changing this seems reasonable to me. In general, we shouldn't make breaking changes to the format, but since it would be to match what go fmt already does, I think I agree that it's a net win.

@kevinburke
Copy link
Contributor Author

@griesemer - thoughts? Maybe I should just submit a patch?

@griesemer
Copy link
Contributor

@kevinburke My apologies but I haven't looked into this yet as it doesn't seem urgent. I will look into this in the next couple of days.

@griesemer
Copy link
Contributor

@kevinburke I agree there's a discrepancy.

The intention behind the printer's default settings was to produce tab-aligned output. gofmt's fine-tuning (using blanks for all alignment but indentation) came later. (There was a long period of trial and error with gofmt's settings before it eventually settled to what we have now).

I'd be somewhat reluctant to change the settings here simply because I don't know how much code depends on what's here now.

Instead, I'd recommend that you use go/format (format.Node) instead of printer.Fprint. It has the same signature and it supposed to produce gofmt-compliant output.

If you're ok with this answer I'll close this issue. Otherwise, please raise your objections. Thanks.

@bradfitz
Copy link
Contributor

Maybe documentation somewhere saying a short version of that?

@gopherbot
Copy link
Contributor

CL https://golang.org/cl/29790 mentions this issue.

@golang golang locked and limited conversation to collaborators Sep 26, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge
Projects
None yet
Milestone
Go1.8
Development

No branches or pull requests
6 participants
@bradfitz @rakyll @quentinmit @kevinburke @griesemer @gopherbot