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
Single import declarations should be grouped with parentheses #97
Comments
My reasoning is that it does not, because it's extremely rare to have files that will only ever need one import. In most cases, the file will need at least one other import sooner rather than later, so keeping the parentheses makes diffs smaller and easier to review. There might be cases where the author truly will only ever need one import, but I think in that very rare case, they can simply use the single-line form manually. I think this scenario is so rare that the downsides mentioned above greatly outweigh it. In practice, I've found that the "code churn" logic above does not apply to var/const/type/func blocks, because the vast majority of Go files have multiple of them. For example, most developers would use a new |
Won't good programmers split up their Go code into lots of files, just like in any other programming language? For example, I just did a grep in my most-starred GitHub project for files that had only 1 import (e.g.
Ok. For what its worth, I agree with you. ❤️ However, I do think that
into:
(Currently, it just leaves the code alone and won't transform it.) |
I will change the title of the issue accordingly. =) |
Maybe you prefer a higher number of smaller files? From my own experience, most Go files have at least 3-5 imports, since the vast majority of files will need imports like fmt or strings. I guess we could debate to what degree "rare" or "majority" are true, but if it's common for people to want to add imports to existing files (e.g. see goimports), then I don't think we should make gofumpt cause more diff churn.
That's an interesting idea, but I still think I disagree on the grounds that, if the user really avoided the parentheses, the tool shouldn't really fight that. Maybe a file will really only ever need one import, like "unsafe". It's also somewhat common to just have |
Allow me to discuss this a bit further in order to convince you. =) Consider the following hypothetical program: func main() {
fmt.Println("hello world")
} After hitting save in my IDE, import "fmt"
func main() {
fmt.Println("hello world")
} Now, say that I add a line with import (
"fmt"
"math"
)
func main() {
fmt.Println("hello world")
fmt.Println(math.Abs(-1))
} Now, say that I don't need the import (
"fmt"
)
func main() {
fmt.Println("hello world")
} My new file is now different than my original file. This seems quite terrible to me - won't it cause noise in git diffs? e.g. "Hey Bob, I noticed that you added parenthesis to this And won't it cause problems with tools that track file changes, and so forth? I am of the opinion that a formatting tool should always give consistent results on the very first run. Getting different output after running the tool twice should probably be considered a bug. (I acknowledge that in this specific hypothetical, the bug I am describing is the consequence of Furthermore, it is important to note that in the above hypothetical, the programmer in question has made no deliberate stylistic decisions about whether they wanted
Ok, perhaps it would make sense to have an exception for this explicit case? In my estimation, this cgo convention seems less important than the problem I am describing above. |
I think this is a good summary of what I think here.
An exception to the rule is not a showstopper, but it does make the rule a bit less consistent and potentially less valuable. I generally agree with you that, at a high level, it's weird that adding and removing a second import results in extra verbosity, even when using gofumpt. I'm still undecided that it is worth it to be extra strict here. On one hand, it would be helpful for "finished" files that will likely not gain more imports. But for files which are still a work in progress or will gain a significant amount of code, removing the parentheses only to have them added again later could annoy the user with unnecessary git commit diffs. I'll leave this open for a little while, if that's okay, because this doesn't seem like a clear "yes" or "no" to me. I think the logical next step would be to implement this, run it over a large corpus of Go code (e.g. half a dozen big Go projects) and look at the results. Do those changes look like reasonable things that upstream would approve and merge? You could even send a few real PRs and get real feedback from their responses. If they say "this is code churn for little reason", you could mention that it could soon be part of an automated tool, so the diff churn would only be temporary. |
I forgot that we re-purposed this proposal. I do fully object to always adding parentheses. That would add two extra lines to code that is otherwise fine, and I expect most maintainers would reject that unless they're already big fans of gofumpt :) What I said above about "I'm on the fence and we could experiment about this" is about removing the paremtheses when there is only one import in a group.
For what it's worth, I would not rule out this use case entirely. It's not common, but I do write or update import declarations from time to time. |
Fair point. I'd push back a little bit on how most maintainers would react to some extra newlines. In my experience, it is fairly normal for formatters to insert extra new lines for the purposes of consistency (having used a lot of Prettier in TypeScript and black in Python). In fact, being wary of spurious extra new lines is a great argument for not implementing #70, since there are a lot of cases where end-users might manually want a few long lines that are not split up into a huge number of smaller lines - but we don't seem to have a problem with wanting to merge this. I think that if maintainers are using I guess my overall point is that if you think that people would be scared off by my relatively tiny proposal, why on earth would we implement #70? |
Turns out that upstream has thought about this in the context of goimports as well; see golang/go#18051 (which I commented on, and forgot about!) and golang/go#48176. The consensus in goimports, as implemented currently, seems to be that tooling should keep parentheses if they are already there, or use them when inserting imports, but it shouldn't add parentheses to a single import which doesn't need to be modified. I'm starting to change my mind - I now lean slightly in favor of enforcing parentheses for single imports, because as time goes on I value consistency and straightforward diffs more. I also don't think my earlier point on code churn really stands, because if a project consistently uses gofumpt, they would only have this kind of churn when they first adopt or apply gofumpt. The code will end up being more verbose, with single imports taking three lines rather than one, but overall I think Go files with a single import are relatively rare, and the majority do end up gaining more imports over time. We should definitely not touch |
I'd be in favor of always using parens for consistency. It's a bit weird currently how you can open up a file with a single import on one line, hack for a bit, and end up with the same code but with parentheses. This happens if you add some new code that pulls in a new import and then later remove that dependency. Or the reverse: if the file starts with parentheses, and your hacking takes it to an intermediate state with 0 imports, and then you add back one import, now the new state will be the same as the original but without parens. IOW, at each state, gofumpt "respects what's already there", as intended, but the net effect that you experience as part of a typical "edit, gofumpt, edit, gufmpt" code-authoring cycle is that parentheses for single imports can be added or removed seemingly-arbitrarily. |
Currently, gofumpt has a rule of "Single var declarations should not be grouped with parentheses". This seems like a sensible rule.
However, doesn't the same logic also apply to import statements?
For example, using a modified version of the code in the README.md for the aforementioned rule:
The text was updated successfully, but these errors were encountered: