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
x/tools/cmd/goimports: return zero exit code if an offense was resolved #39316
Comments
Return zero if goimports was successfully able to modify files in place even if -l is set to list the changed files. Fixes: golang/go#39316
Change https://golang.org/cl/235526 mentions this issue: |
Can this be put behind a new flag? This is a breaking change and not an intuitive one to figure out what happened. |
This reverts a breaking change (returning non-zero on |
@jthurman42 I understand, my comment wasn't clear. The pr should be reverted OR put behind a flag as an option vs being the default behavior of -l |
@dramich Ah gotcha. You'll want to comment here: https://go-review.googlesource.com/c/tools/+/235526/ as the Github issues do not appear to be monitored (and do not sync to that thread 😢 ) |
On the contrary, GitHub issues are the canonical locations for discussion and decisions. The code review comments are for discussion of the code itself primarily. |
Given the back and forth on this issue I'm concluding that changing the behavior in any way is too much. I'll roll back the original CL. If someone wants to do an analysis of the various use cases and propose a backwards-compatible flag, we can discuss that. But overall it seems like anyone who cares can just run goimports twice. Sorry for the trouble and delay. |
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
As part of our make process, we allow goimports to automatically fix import formatting issues. The behavior changed with #39032 in an unexpected way. Returning non-zero makes perfect sense except when the imports were automatically resolved.
What did you expect to see?
List of modified files, build process continues
What did you see instead?
List of modified files, build process terminates (exit code 1 from
goimports
)The text was updated successfully, but these errors were encountered: