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

x/tools/cmd/goimports: New feature, option to always use parentheses #48176

Open
mcandre opened this issue Sep 3, 2021 · 7 comments
Open

x/tools/cmd/goimports: New feature, option to always use parentheses #48176

mcandre opened this issue Sep 3, 2021 · 7 comments
Labels
FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Milestone

Comments

@mcandre
Copy link

mcandre commented Sep 3, 2021

I would love an option to force goimports to always use parentheses, even for .go source files with only one import line.

By always using parentheses, the diff's between refactoring commits will be shorter. It's the same reason we use trailing commas on Go collections.

@gopherbot gopherbot added the Tools This label describes issues relating to any tools in the x/tools repository. label Sep 3, 2021
@gopherbot gopherbot added this to the Unreleased milestone Sep 3, 2021
@seankhliao
Copy link
Member

cc @heschi

@cherrymui cherrymui added the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Sep 3, 2021
@findleyr
Copy link
Contributor

findleyr commented Sep 3, 2021

Related, one thing that I run into from time to time is the following asymmetry:

  • if I have two imports and goimports removes one, it will preserve the parentheses
  • if I have one (parenthesized!) import which goimports replaces with another, goimports removes the parentheses
  • if I have no imports and goimports adds one, it chooses no parentheses

Not sure whether we want to change goimports behavior at this point, but I would prefer if goimports would choose a single canonical format for the case of one import, though I don't particularly care which format (I suppose I do mildly prefer parenthesized). This proposal would address that.

@josharian
Copy link
Contributor

We made a decision some time back that goimports should standardize on always using parentheses, at all times, so that diffs are single-line. (On my phone, don't have an issue number handy.) If goimports removes parentheses or adds a parens-less import, that is a bug and should be fixed.

I'd be happy to review CLs that fix such bugs. Note that this code is particularly delicate and error-prone and has a long history of problems. CLs should include extensive, inventive tests. (There are many already to take inspiration from.)

@heschi
Copy link
Contributor

heschi commented Sep 3, 2021

@mcandre: at this point, we're not going to add new options to goimports without an extremely compelling requirement. This doesn't qualify. Anyone is welcome to make their own version that behaves the way you describe; that should be easy to do as a postprocessing step around imports.Process.

@findleyr: regarding fixing the asymmetry: I agree that the behavior seems odd at first glance. I suspect that this has to do with not destroying comments -- in general, most odd goimports formatting decisions do -- but I'm not sure without doing some investigation.

@josharian I would be very curious to see a citation for that decision if you can find it. Non-parenthesized single imports has been a tested behavior since before 2013, unless I'm missing something. And in general I'd be loath to make different versions of goimports fight over whether to parenthesize or not, and also to make people reformat their code bases to stay goimports-clean.

@heschi
Copy link
Contributor

heschi commented Sep 3, 2021

...all that said, I suppose it's safe to change the behavior of adding a single import be parenthesized if we want to, just not to have it change a single existing import.

@josharian
Copy link
Contributor

I dug, and apparently the consensus I recalled was...my opinion. :P It is discussed in #18051. In any case, I agree that goimports shouldn't change any import blocks unless it needs to change the set of imports. But once we are modifying/creating a set of imports, I think we should always switch to the factored version.

@gopherbot
Copy link

Change https://golang.org/cl/355250 mentions this issue: cmd/compile: some fixes in type substituter for Instantiate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
FeatureRequest NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Tools This label describes issues relating to any tools in the x/tools repository.
Projects
None yet
Development

No branches or pull requests

7 participants