-
-
Notifications
You must be signed in to change notification settings - Fork 114
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
break long lines when it's appropriate to do so #2
Comments
Thanks for the interest! This is indeed part of my grand plan :) I've left this feature out of the initial implementation, simply because it's one of the tricky ones. A number of decisions must be taken:
The closest existing tool is https://github.com/walle/lll, which counts runes (characters), and defaults to a maximum of 80. My current thinking is to start small and conservative. That is, use a simple large limit like 120 runes, and only add newlines in non-controversial places, like between expressions in an expression list (such as your example). If we're happy with that, we can look at more complex cases, like splitting up strings and comments, or dealing with nested expressions. I think counting runes is the only sane default we can use. Counting bytes is just far too basic. Measuring width via https://godoc.org/golang.org/x/text/width would work, but only for monospace fonts that happen to agree with that package.
So I think it's fair to follow gofmt here, for the sake of simplicity and consistency. Also cc @rogpeppe, who has thought about breaking super long lines for a while. |
On the other hand, forcing this behavior by default could mean that gofumpt would be less of a drop-in replacement, if the line-breaking is too agressive. I personally never write lines over 100-120 characters long, but sometimes I do go over 80 when declaring functions or writing comments. I'd have to experiment with large amounts of Go code. |
golang/go#11915 outlines more or less my concerns above. I see this project as a way to experiment with these features. golang/go#8146 is more interesting. It turns out that
Looks like
So, in a way, |
Yeah, I have added prettier to some very old and very large JavaScript / TypeScript projects, and the We can probably assume that all Go code is at least |
Please make this opt-in only :) ... multiline function definitions are ugly IMHO. |
Sorry, but as I commented in another issue, it's unlikely that gofumpt will ever have any options of its own - just like gofmt doesn't have any formatting optins. |
Yes, please no options. It is either in or not. Adding options is a whole new can of worms that Go does not need. |
+1 for multiline function definitions and calls. I understand that you don't like options, I assume that includes different versions of the tool? Maybe something behind a build tag and people who really don't want this can build the tool with something like an |
Sorry, I don't follow. The plan is to try with a large limit like 120 first, which would apply to all files equally. Conservatively, it would likely ignore lines that can't be split into shorter ones. |
@mvdan I apologize, I should have explained better. Instead of having a file or flags to manage what the formatting looks like, you could use build flags to change which of the // +build splitLongFunctions
package internal
func init() {
// add splitLongFuncFumpter to the list of fumpters to run
}
type splitLongFuncFumpter struct {
// ...
} You could then install the tool via The more I think about this the more I understand why it's not going to work. It is a pretty weird approach but at least allows you to build distinct formatting tools for each project. |
I was going to ask you about this very feature at Go meetup! I am in favour of this, this is the main thing I miss from Prettier and Rustfmt when writing Go. I often have long function decls - partly because I don't like the chaos of single letter variables in functions that are longer than a few lines and also descriptive names are self-documenting! Because of this, I often manually split declarations, struct instantiations and calls. Another neat thing about Prettier that gives the user a little control (if you're into that) is the ability to choose between short object declarations being single lines or split. For example: let x = { a: A, b: B }; Prettier will leave this alone as it doesn't violate the line limit however if I wanted to force this declaration to span multiple lines, perhaps for clarity or the ability to write a comment after each field, I could add a newline after the first let x = {
a: A, b: B }; And upon formatting, Prettier will see this as a hint that you wish to declare this across multiple lines and as a result, format it to: let x = {
a: A,
b: B,
}; In comparison, Rustfmt doesn't permit this and has a single possible format (as far as I can tell) for all code, as in, there's nothing the user can do to the source in order to "hint" to the formatter to format something differently. I like this approach too because it completely removes formatting from the burden of the user - which, as you've pointed out with gofumpt, gofmt does not do adequately. Anyway, food for though, I am looking forward to a line-length formatting feature! As for length, I'd vote for 100 or 120, 80 imo too oldschool and is way too narrow even on a tiny laptop with two editors open. |
I agree with @Southclaws this would be a great feature to have, I can understand that Perhaps a sensible default such as 100 or 120 could be added? If anyone insists on having something different they can add in a build flag to override the default settings. |
Thanks all for the comments. This is definitely something I want to try; it's just a matter of finding the time to properly write it and test it. |
Looking forward to testing this feature! Having that feature would be greatest impact on the go fmt ecosystem, to be honest, it is a lot of frustration to manually format Go code after working with Java for some time where code formatting is very powerful, for example: |
Anyone working on this? |
@remorses it's still very much planned, but it's a non-trivial feature and I haven't found the time to properly work on it. I have a lot of side projects and limited spare time :) Besides bringing up ideas and sending patches, there's also the option of sponsoring through github, which allows me to spend more time on projects like gofumpt. I believe they now have a feature where you can say the reason behind the sponsorship, so that the receiver can understand where they should be spending more time. Of course, gofumpt will always be free software, and the feature will likely be done sooner or later. But you should understand that contributing and sponsoring are the only two ways to move things along. |
I put together a rough first cut of this feature in #70. Please try it out and provide feedback. To repeat the design choices here, at least for the initial implementation:
The heuristic isn't documented, but you can get a rough idea of how it behaves by looking at the test cases added in the PR. |
This is awesome @mvdan thank you :D ❤️ Is the way func definitions and calls are split final? Currently if err := f(argument1, argument2, argument3, argument4, argument5, argument6, &someComplex{argument7, argument8, argument9}); err != nil {
panic(err)
} becomes (1) if err := f(argument1, argument2, argument3, argument4, argument5, argument6, &someComplex{
argument7, argument8, argument9}); err != nil {
panic(err)
} Is this an acceptably readable result? Possible alternatives would be I guess something like (2) if err := f(
argument1, argument2, argument3, argument4, argument5, argument6,
&someComplex{
argument7,
argument8,
argument9,
},
); err != nil {
panic(err)
} or (3) if err := f(
argument1,
argument2,
argument3,
argument4,
argument5,
argument6,
&someComplex{
argument7,
argument8,
argument9,
},
); err != nil {
panic(err)
} My personal preference is the last one (3), but I understand not many people like that. ps. Same thing with the func definitions, though I agree that returns should be kept together. Kubernetes does this nicely for func definitions which I really like. func NewCertificateController(
name string,
kubeClient clientset.Interface,
csrInformer certificatesinformers.CertificateSigningRequestInformer,
handler func(*certificates.CertificateSigningRequest) error,
) *CertificateController { (source) |
I agree with @geoah , 3 is my preference as well. |
I don't think we will do 3; there is nothing wrong with a call like:
Similarly, I think 2 would still be too aggressive. There's nothing wrong with some parameters being on the same line as the called function, like in the snippet above. The only case where we're stricter is with composite literals. 1 is definitely wrong, though. The formatter should be enforcing a newline before the closing curly brace if there is a newline after the opening curly brace. The current WIP patch might require you to run the formatter twice to get that, but I'll fix it. It's fine if a project has a strong preference for one parameter per line, but that rule is far too aggressive for a generic tool. And if a project uses that style, gofmt and gofumpt shouldn't break it either. |
If |
FWIW personally I think that looks ugly, and it's easy to miss arguments; when I reformat code like that, I either choose all args on one line or all args on separate lines. I'd be interested to see how the different approaches end up looking when run on real code. |
This sounds like a good guideline to me. Anything in between is ugly and harder to read. |
I find this really hard to read, finding where the function opens, how many arguments there are on each line and where it closes is really difficult compared to a single line or split fully. |
Please remember that the first iteration of this feature needs to be very conservative. It seems to me like forcing arguments to all be on separate lines is exactly the opposite of conservative. Let's go step by step. If a conservative heuristic goes well and people seem to like it, we can push it further in the future. For example, by requiring that multi-line parameter and argument lists have each element in a separate line. We already enforce that rule for composite literals. I guess I could be convinced that such a stricter format would be a good thing, but I don't see it clearly right now, and I work with a lot of code that would be heavily modified by such a rule. Hence why I strongly prefer to leave it for a future incremental step. |
It looks like this didn't make it into the 0.1.1 release. Do you have a time estimate when the next release might be? I'm going to push golangci-lint to update the version of gofumpt they use as soon as there's a release out with this in it. :-) |
This is experimental, so it's on purpose that it isn't shipping (and enabled) with stable releases just yet. |
I've been trying to keep the env var on at all times, and I've been finding it's a tiny bit too aggressive - especially on indented code. For example, it did this change: typeInfo := info.TypeOf(x)
- if typeInfo != types.Typ[types.String] && typeInfo != types.Typ[types.UntypedString] {
+ if typeInfo != types.Typ[types.String] &&
+ typeInfo != types.Typ[types.UntypedString] {
return true
} This definitely feels wrong. The indented line does reach to column 110 if we count tabs as 8 columns, but the non-indented line is just 86 characters, and we end up with two tiny lines of 41 and 44 non-indented characters. The new format is not wrong per se, but I lean towards the old format, and I don't think there's anything wrong with it. I think it falls under "goes over the limit, but splitting the line isn't better". I'll probably tweak the heuristic to be a bit less aggressive on indented lines. |
Agreed, something about putting the second condition on the next line right next to the
into this:
which works for me, but only because of the way the parens are used here, which is optional and completely uncommon in Go code |
That's also a good point. Splitting one line into two, when two lines adds ambiguity, is not good. The example I shared would need another extra newline to become non-ambiguous again. I worry that it might be tricky to determine "will the new format look ambiguous", though. Though I still think the line on its own should be left alone, even if the ambiguity wasn't an issue. |
Another example, this time in
becomes:
|
This comment has been minimized.
This comment has been minimized.
I also think gofumpt should make it clear what it's optimizing the code's presentation for: editors, diffs, just the
I agree that following upstream is the best approach for the project, so I suppose the scope could be clarified to "optimizing display for simple diffs or pagers, assuming all code points have equal width". That being said, I'd like to see support for code points with different widths. Some of the people who worked on https://github.com/psf/black have had lots of discussions on line-wrapping (stemming from a decision to break from PEP-8's 79/80-col recommendation) and may be able to share thoughts. |
When will this be released as stable? Great job just tested btw. |
for now using https://github.com/segmentio/golines as an alternative. |
@mvdan Thank you for the awesome feature! Could longLineLimit be configurable? |
Hi @mvdan,
|
Very excited about this feature. I've tested it against a few projects and it's worked great for me. |
In the past I've written Dart with dartfmt. I definitely miss the auto-line wrapping when writing Go. Especially the amount of noise, that not having auto-line wrapping, adds to MR discussions. Cheers for working on this! Regarding wrapping long argument and collection lists, dartfmt's approach is to wrap these differently depending on the presence of a trailing comma (been a while since I wrote any, so hopefully my memory is correct). So this: func foo(a string, b int, c float,) {
...
} Becomes: func foo(
a string,
b int,
c float,
) {
...
} And without the trailing comma: func foo(reallyReallyReallyReallyReallyLongName string, otherReallyReallyReallyReallyReallyLongName int, yetAnotherreallyReallyReallyReallyReallyLongName float) {
...
} Becomes: func foo(reallyReallyReallyReallyReallyLongName string, otherReallyReallyReallyReallyReallyLongName int,
yetAnotherreallyReallyReallyReallyReallyLongName float) {
...
} Seems to work quite well in practise. Hopefully Go's simpler syntax will make this easier than implementing "The Hardest Program I’ve Ever Written". See: https://journal.stuffwithstuff.com/2015/09/08/the-hardest-program-ive-ever-written/ |
in vscode it can be set in settings.json with:
|
I'm happy to have ability to split long lines, thank you |
Thank you for working on this. I would love to have flags; this is already a subset of I also wanted to express my support for lines 80 characters long, and the following format, shared above by @geoah:
I find having parameters and arguments on their own line, separate from the parentheses, easier to read and refactor. |
In Neovim with lsp-config, this feature can be enabled with:
|
Just curious if this feature is on parity with golines? It is ideal if we only needed to use gofumpt as it makes editor configuration much simpler (since gopls supports gofumpt). 😄 |
Howdie, love your work, and just stumbled across this which also looks awesome :)
I use prettier with JavaScript, and
cargo fmt
with Rust, and they both automatically (and consistently) break up long linesExample
func
:gofmt
doesn't do this, but it also doesn't undo it where I've done this manually, so at least this particular example isgofmt
-compatibleThe text was updated successfully, but these errors were encountered: