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

revamp //gofumpt:diagnose for real use cases #239

Merged
merged 1 commit into from
Aug 3, 2022
Merged

Conversation

mvdan
Copy link
Owner

@mvdan mvdan commented Jul 27, 2022

(see commit message)

@mvdan mvdan requested a review from Oiyoo July 27, 2022 16:03
@mvdan mvdan force-pushed the diagnose-version branch from 74a16a6 to 08d36aa Compare July 27, 2022 16:04
@mvdan
Copy link
Owner Author

mvdan commented Jul 27, 2022

Drat, Go 1.17. I will temporarily disable the VCS stuff until Go 1.19 is released and we drop 1.17.

@mvdan
Copy link
Owner Author

mvdan commented Jul 27, 2022

Hm, that would be a larger refactor than I thought. I'll instead wait for 1.19 to be out, then drop 1.17, then rebase this to get it green.

Right now, it simply reports "(devel)" as the gofumpt version when
gofumpt is used as a library via the format package,
which is the case for many users like golangci-lint.
To properly test this behavior, we add a gofumpt-external program
which simply formats stdin with one option and prints to stdout.
We `go install` this tool, which depends on a released gofumpt version.

It also does not report the Go version, which is important as we rely on
packages like go/printer, which can influence our behavior.
Similar to the case above, we `go install` a released gofumpt version.

It also does not use VCS information when it is the main module;
add that as well, along with a test, borrowed from mvdan.cc/garble.

While here, make the output easier to read by adding "version" and
"flags" words as descriptions.

Finally, we remove version.Print, which was an unnecessary wrapper.

The following PR will update the released version of gofumpt being used,
meaning that the "released" and "external" tests will pick up the improvement.
@mvdan mvdan force-pushed the diagnose-version branch from 08d36aa to d42629d Compare August 3, 2022 11:37
@mvdan
Copy link
Owner Author

mvdan commented Aug 3, 2022

Rebased, should be green now.

@mvdan mvdan merged commit 70d7433 into master Aug 3, 2022
@mvdan mvdan deleted the diagnose-version branch August 3, 2022 14:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants