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

break long lines when it's appropriate to do so #2

Open
jokeyrhyme opened this issue Mar 31, 2019 · 60 comments
Open

break long lines when it's appropriate to do so #2

jokeyrhyme opened this issue Mar 31, 2019 · 60 comments
Labels
enhancement New feature or request

Comments

@jokeyrhyme
Copy link

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 lines

Example func:

func somethingTooLong(ctx context.Context, longArgument string, anotherOne func(time.Duration) bool) (int, error) {
	// foo
}
func somethingTooLong(
	ctx context.Context,
	longArgument string,
	anotherOne func(time.Duration) bool
) (int, error) {
	// foo
}

gofmt doesn't do this, but it also doesn't undo it where I've done this manually, so at least this particular example is gofmt-compatible

@mvdan
Copy link
Owner

mvdan commented Mar 31, 2019

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:

  • Do we want this to be configurable? Following gofmt, I'd very much rather not add a flag.
  • How do we measure line length? A number of characters, a number of bytes, the line width when printed on screen?
  • If we break lines after a limit, what should that limit be? 80 is probably too aggressive. 100? 120?
  • Do we also want to break long strings and long comments?

The closest existing tool is https://github.com/walle/lll, which counts runes (characters), and defaults to a maximum of 80. rustfmt seems to enforce a configurable limit, defaulting to 100: https://github.com/rust-lang/rustfmt/blob/master/Configurations.md#max_width

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. gofmt aligns comments, so it must also measure the length of lines, for which it uses https://golang.org/pkg/text/tabwriter/:

The Writer assumes that all Unicode code points have the same width; this may not be true in some fonts or if the string contains combining characters.

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.

@mvdan
Copy link
Owner

mvdan commented Mar 31, 2019

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.

@mvdan
Copy link
Owner

mvdan commented Mar 31, 2019

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 gofmt does break up long func lines, if they are "complex" enough:

const maxSize = 100
if headerSize+p.bodySize(b, maxSize) <= maxSize {

Looks like headerSize and bodySize are just approximations of the number of characters it will take to print each bit of code:

nodeSize determines the size of n in chars after formatting. The result is <= maxSize if the node fits on one line with at most maxSize chars and the formatted output doesn't contain any control chars. Otherwise, the result is > maxSize.

So, in a way, gofmt already enforces certain line length limits. I think people are happy with 100 for top-level function declarations, since there's zero indentation and there's little reason to make them long. So this tells us that we definitely want at least 100 as a global limit.

@mvdan mvdan changed the title thoughts on line-breaks? consider breaking long lines Mar 31, 2019
@jokeyrhyme
Copy link
Author

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.

Yeah, I have added prettier to some very old and very large JavaScript / TypeScript projects, and the git diff can be pretty intimidating

We can probably assume that all Go code is at least gofmted, though, which is better than the average JavaScript project :P

@andrewrynhard
Copy link

Please make this opt-in only :) ... multiline function definitions are ugly IMHO.

@mvdan
Copy link
Owner

mvdan commented Aug 17, 2019

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.

@omeid
Copy link

omeid commented Aug 25, 2019

Yes, please no options.

It is either in or not. Adding options is a whole new can of worms that Go does not need.

@geoah
Copy link

geoah commented Oct 1, 2019

+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 !lll build tag?

@mvdan
Copy link
Owner

mvdan commented Oct 2, 2019

different versions of the tool

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.

@geoah
Copy link

geoah commented Oct 2, 2019

@mvdan I apologize, I should have explained better.
I was just suggesting a different way to do configuration.
I understand you probably are not interested in this either but I really would love to see splitting of long functions getting into gofumpt. :D

Instead of having a file or flags to manage what the formatting looks like, you could use build flags to change which of the fumpters get loaded.

// +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 go get -flags splitLongFunctions to include this functionality.

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.

@Southclaws
Copy link

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.

@gnu-user
Copy link

I agree with @Southclaws this would be a great feature to have, I can understand that gofumpt should have a feature for formatting long lines, especially when working with teams on larger code bases, inconsistencies with regards to line length/wrapping can be a pain.

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.

@mvdan
Copy link
Owner

mvdan commented Nov 11, 2019

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.

@kovetskiy
Copy link

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:
https://github.com/palantir/palantir-java-format
https://github.com/google/google-java-format

@remorses
Copy link

remorses commented May 8, 2020

Anyone working on this?

@mvdan
Copy link
Owner

mvdan commented May 8, 2020

@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.

@mvdan
Copy link
Owner

mvdan commented May 12, 2020

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:

  • It needs to be conservative. If any change seems annoying or makes code less readable, that's a bug.
  • It can't be configurable, since we have no flags on top of plain gofmt.
  • We can only insert newlines. Heavier refactors like splitting string literals are not allowed.

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.

@geoah
Copy link

geoah commented May 12, 2020

This is awesome @mvdan thank you :D ❤️
I'll post this here so not to pollute the PR with discussions.

Is the way func definitions and calls are split final?
If not, do you have an idea of how you'd like them to look like?


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)

@andrewrynhard
Copy link

I agree with @geoah , 3 is my preference as well.

@mvdan
Copy link
Owner

mvdan commented May 12, 2020

I don't think we will do 3; there is nothing wrong with a call like:

fmt.Printf("some %v format %v string etc etc...", arg1, arg2,
    arg3, arg4, arg5, arg6)

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.

@andrewrynhard
Copy link

If gofumpt can't be configurable, this means that the convention of 1 would be imposed and there will be no way to disable this particular functionality of gofumpt?

@rogpeppe
Copy link

I don't think we will do 3; there is nothing wrong with a call like:

fmt.Printf("some %v format %v string etc etc...", arg1, arg2,
    arg3, arg4, arg5, arg6)

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.

@andrewrynhard
Copy link

I either choose all args on one line or all args on separate lines.

This sounds like a good guideline to me. Anything in between is ugly and harder to read.

@Southclaws
Copy link

I don't think we will do 3; there is nothing wrong with a call like:

fmt.Printf("some %v format %v string etc etc...", arg1, arg2,
    arg3, arg4, arg5, arg6)

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.

@mvdan
Copy link
Owner

mvdan commented May 12, 2020

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.

@StevenACoffman
Copy link

Woohoo! We currently, manually use segmentio/golines for this, if there is anything useful to be gleaned from that codebase, by the way.

@csilvers
Copy link

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. :-)

@mvdan
Copy link
Owner

mvdan commented Apr 5, 2021

This is experimental, so it's on purpose that it isn't shipping (and enabled) with stable releases just yet.

@mvdan
Copy link
Owner

mvdan commented Apr 29, 2021

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.

@jokeyrhyme
Copy link
Author

jokeyrhyme commented Apr 29, 2021

Agreed, something about putting the second condition on the next line right next to the if body irks me a bit

prettier turns this:

if (isThisAReallyLongVariableName === 123 && doReallyLongNamedCheck() === true) {
  return true
}

into this:

if (
  isThisAReallyLongVariableName === 123 &&
  doReallyLongNamedCheck() === true
) {
  return true;
}

which works for me, but only because of the way the parens are used here, which is optional and completely uncommon in Go code

@mvdan
Copy link
Owner

mvdan commented Apr 29, 2021

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.

@jokeyrhyme
Copy link
Author

Another example, this time in rustfmt (Rust is like Go in that if conditions do not require parens):

    if isThisAReallyReallyReallyReallyReallyLongVariableName == 123 && doReallyReallyReallyLongNamedCheck() == true {
        return true;
    }

becomes:

    if isThisAReallyReallyReallyReallyReallyLongVariableName == 123
        && doReallyReallyReallyLongNamedCheck() == true
    {
        return true;
    }

@mvdan mvdan removed this from the v0.2.0 milestone May 20, 2021
@gideongrinberg

This comment has been minimized.

@Seirdy
Copy link

Seirdy commented Jan 23, 2022

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.

I also think gofumpt should make it clear what it's optimizing the code's presentation for: editors, diffs, just the $PAGER, etc. Counting graphemes would optimize for most graphical editors (and a few console-based ones); counting cell width would optimize for pretty diffs.

gofmt aligns comments, so it must also measure the length of lines, for which it uses https://golang.org/pkg/text/tabwriter/:

The Writer assumes that all Unicode code points have the same width; this may not be true in some fonts or if the string contains combining characters.

So I think it's fair to follow gofmt here, for the sake of simplicity and consistency.

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.

@1Mark
Copy link

1Mark commented May 14, 2022

I've merged the long-lived PR into master. Note that the feature is not enabled by default just yet, because I want to get some early feedback first.

If you would like to help:

go install mvdan.cc/gofumpt@master

Then run it with the special env var:

GOFUMPT_SPLIT_LONG_LINES=on gofumpt -l -w .

If you find a bug or a regression, or a case where a line was split when it clearly shouldn't have been, please leave a comment here. This issue will remain open until the feature is enabled by default. Thank you!

When will this be released as stable? Great job just tested btw.

@1Mark
Copy link

1Mark commented May 14, 2022

for now using https://github.com/segmentio/golines as an alternative.

@OrangeFlag
Copy link

@mvdan Thank you for the awesome feature!

Could longLineLimit be configurable?

@kevinjcliao
Copy link

Hi @mvdan,
Thanks so much for your work with gofumpt long lines! While testing the feature on my codebase, I ran into a panic. Do you have any idea what's going on here? Thanks.

panic: negative length [recovered]
	panic: negative length

goroutine 1 [running]:
golang.org/x/tools/go/ast/astutil.Apply.func1()
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:49 +0x89
panic({0x1209440, 0x128f368})
	/usr/local/go/src/runtime/panic.go:1038 +0x215
mvdan.cc/gofumpt/format.(*fumpter).splitLongLine(0xc000375000, 0xc0000653f0)
	/Users/kevin/go/pkg/mod/mvdan.cc/gofumpt@v0.2.0/format/format.go:767 +0x618
mvdan.cc/gofumpt/format.(*fumpter).applyPre(0xc000375000, 0xc0000653f0)
	/Users/kevin/go/pkg/mod/mvdan.cc/gofumpt@v0.2.0/format/format.go:317 +0x3e
mvdan.cc/gofumpt/format.File.func1(0xc0000653f0)
	/Users/kevin/go/pkg/mod/mvdan.cc/gofumpt@v0.2.0/format/format.go:104 +0x3a
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x12941a0, 0xc0002ddec0}, {0x124789e, 0xc0002ddec0}, 0x1205880, {0x12944e8, 0xc0002e1760})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:200 +0x202
golang.org/x/tools/go/ast/astutil.(*application).applyList(0xc0000653e0, {0x12941a0, 0xc0002ddec0}, {0x124789e, 0x4})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:479 +0xae
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294060, 0xc0002ddf00}, {0x12477a3, 0x0}, 0x129ce00, {0x12941a0, 0xc0002ddec0})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:272 +0x219e
golang.org/x/tools/go/ast/astutil.(*application).applyList(0xc0000653e0, {0x1294060, 0xc0002ddf00}, {0x12477a3, 0x3})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:479 +0xae
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294510, 0xc0002e2080}, {0x12478e2, 0x8}, 0xc0001e34a0, {0x1294060, 0xc0002ddf00})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:336 +0x7ed
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294150, 0xc0002c3dd0}, {0x12478f2, 0xc0002e0ea0}, 0x129ce00, {0x1294510, 0xc0002e2080})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:354 +0x416
golang.org/x/tools/go/ast/astutil.(*application).applyList(0xc0000653e0, {0x1294150, 0xc0002c3dd0}, {0x12478f2, 0x4})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:479 +0xae
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294510, 0xc0002e20c0}, {0x12478a6, 0x8}, 0xc0001e3290, {0x1294150, 0xc0002c3dd0})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:351 +0x310
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294150, 0xc0002c3e00}, {0x12478f2, 0xc0002c3bc0}, 0x129ce00, {0x1294510, 0xc0002e20c0})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:356 +0x49b
golang.org/x/tools/go/ast/astutil.(*application).applyList(0xc0000653e0, {0x1294150, 0xc0002c3e00}, {0x12478f2, 0x4})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:479 +0xae
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294510, 0xc0002e2180}, {0x12478a6, 0x8}, 0xc0001e3108, {0x1294150, 0xc0002c3e00})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:351 +0x310
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294150, 0xc0002c3e30}, {0x12478f2, 0xc0002c3b00}, 0x129ce00, {0x1294510, 0xc0002e2180})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:356 +0x49b
golang.org/x/tools/go/ast/astutil.(*application).applyList(0xc0000653e0, {0x1294150, 0xc0002c3e30}, {0x12478f2, 0x4})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:479 +0xae
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294510, 0xc0002e21c0}, {0x12478a6, 0x8}, 0xc0001e2f10, {0x1294150, 0xc0002c3e30})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:351 +0x310
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294150, 0xc0002c3e60}, {0x12478f2, 0xc0002dcf00}, 0x129ce00, {0x1294510, 0xc0002e21c0})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:356 +0x49b
golang.org/x/tools/go/ast/astutil.(*application).applyList(0xc0000653e0, {0x1294150, 0xc0002c3e60}, {0x12478f2, 0x4})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:479 +0xae
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294510, 0xc0002e2200}, {0x12478a6, 0x8}, 0xc0001e2ed0, {0x1294150, 0xc0002c3e60})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:351 +0x310
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294150, 0xc0002c3e90}, {0x12478f2, 0xc0002dcec0}, 0x129ce00, {0x1294510, 0xc0002e2200})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:356 +0x49b
golang.org/x/tools/go/ast/astutil.(*application).applyList(0xc0000653e0, {0x1294150, 0xc0002c3e90}, {0x12478f2, 0x4})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:479 +0xae
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294510, 0xc0002e2240}, {0x12478a6, 0x8}, 0xc0001e2ec0, {0x1294150, 0xc0002c3e90})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:351 +0x310
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294150, 0xc0002e60f0}, {0x12478f2, 0xc0002c3ad0}, 0x129ce00, {0x1294510, 0xc0002e2240})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:356 +0x49b
golang.org/x/tools/go/ast/astutil.(*application).applyList(0xc0000653e0, {0x1294150, 0xc0002e60f0}, {0x12478f2, 0x4})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:479 +0xae
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294510, 0xc0002e27c0}, {0x12478a6, 0x8}, 0xc0001e2eb8, {0x1294150, 0xc0002e60f0})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:351 +0x310
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294150, 0xc0002e61b0}, {0x12478f2, 0xc0002dc1c0}, 0x129ce00, {0x1294510, 0xc0002e27c0})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:356 +0x49b
golang.org/x/tools/go/ast/astutil.(*application).applyList(0xc0000653e0, {0x1294150, 0xc0002e61b0}, {0x12478f2, 0x4})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:479 +0xae
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294510, 0xc0002e2980}, {0x12478a6, 0x8}, 0xc0001e2a10, {0x1294150, 0xc0002e61b0})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:351 +0x310
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294150, 0xc0002e6360}, {0x12478f2, 0xc0002d7308}, 0x129ce00, {0x1294510, 0xc0002e2980})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:356 +0x49b
golang.org/x/tools/go/ast/astutil.(*application).applyList(0xc0000653e0, {0x1294150, 0xc0002e6360}, {0x12478f2, 0x4})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:479 +0xae
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294420, 0xc0002e6390}, {0x12478a6, 0x18}, 0xc0001e2668, {0x1294150, 0xc0002e6360})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:351 +0x310
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x12943d0, 0xc000326f00}, {0x1247af9, 0xc000326f00}, 0x10, {0x1294420, 0xc0002e6390})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:424 +0x17fd
golang.org/x/tools/go/ast/astutil.(*application).applyList(0xc0000653e0, {0x12943d0, 0xc000326f00}, {0x1247af9, 0x5})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:479 +0xae
golang.org/x/tools/go/ast/astutil.(*application).apply(0xc0000653e0, {0x1294c40, 0xc0003e4000}, {0x124790a, 0x18}, 0x18, {0x12943d0, 0xc000326f00})
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:430 +0xb0d
golang.org/x/tools/go/ast/astutil.Apply({0x12943d0, 0xc000326f00}, 0xc00035d9f8, 0xc00035da10)
	/Users/kevin/go/pkg/mod/golang.org/x/tools@v0.1.8-0.20211102182255-bb4add04ddef/go/ast/astutil/rewrite.go:54 +0x166
mvdan.cc/gofumpt/format.File(0xc0000104c0, 0xc000326f00, {{0xc000092000, 0xc000402000}, 0x0})
	/Users/kevin/go/pkg/mod/mvdan.cc/gofumpt@v0.2.0/format/format.go:152 +0x335
main.processFile({0xc000336f00, 0x20}, {0x0, 0x0}, {0x1290440, 0xc00000e018}, 0x0)
	/Users/kevin/go/pkg/mod/mvdan.cc/gofumpt@v0.2.0/gofmt.go:130 +0x388
main.visitFile({0xc000336f00, 0x20}, {0x1295ee8, 0xc0003cc5c0}, {0x0, 0x0})
	/Users/kevin/go/pkg/mod/mvdan.cc/gofumpt@v0.2.0/gofmt.go:185 +0xf2
path/filepath.walkDir({0xc000336f00, 0x20}, {0x1295ee8, 0xc0003cc5c0}, 0x1256e18)
	/usr/local/go/src/path/filepath/path.go:386 +0x63
path/filepath.walkDir({0xc0004015c0, 0x13}, {0x1295ee8, 0xc000010840}, 0x1256e18)
	/usr/local/go/src/path/filepath/path.go:405 +0x232
path/filepath.walkDir({0x20570268e, 0xc}, {0x1295f20, 0xc000056610}, 0x1256e18)
	/usr/local/go/src/path/filepath/path.go:405 +0x232
path/filepath.WalkDir({0x20570268e, 0xc}, 0x1256e18)
	/usr/local/go/src/path/filepath/path.go:469 +0xb0
main.gofumptMain()
	/Users/kevin/go/pkg/mod/mvdan.cc/gofumpt@v0.2.0/gofmt.go:248 +0x405
main.main()
	/Users/kevin/go/pkg/mod/mvdan.cc/gofumpt@v0.2.0/gofmt.go:195 +0x19

@sevein
Copy link

sevein commented Jun 22, 2023

Very excited about this feature. I've tested it against a few projects and it's worked great for me.

@xxgreg
Copy link

xxgreg commented Jun 22, 2023

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/

And: https://github.com/dart-lang/dart_style

@wiegell
Copy link

wiegell commented Jul 21, 2023

in vscode it can be set in settings.json with:

  "go.toolsEnvVars": {
    "GOFUMPT_SPLIT_LONG_LINES": "on"
  },

@mironnn
Copy link

mironnn commented Dec 28, 2023

I'm happy to have ability to split long lines, thank you

@dwrz
Copy link

dwrz commented Jan 30, 2024

Thank you for working on this.

I would love to have flags; this is already a subset of gofmt, and as long as the output is compatible, I don't see the harm in flags.

I also wanted to express my support for lines 80 characters long, and the following format, shared above by @geoah:

	if err := f(
		argument1,
		argument2,
		argument3,
		argument4,
		argument5,
		argument6,
		&someComplex{
			argument7, 
			argument8, 
			argument9,
		},
	); err != nil {
		panic(err)
	}

I find having parameters and arguments on their own line, separate from the parentheses, easier to read and refactor.

@alexandervantrijffel
Copy link

In Neovim with lsp-config, this feature can be enabled with:

require('lspconfig').gopls.setup({
    cmd_env = {GOFUMPT_SPLIT_LONG_LINES="on"},
    settings = {
        gopls = {
            gofumpt = true
        }
    }
})

@fgimian
Copy link

fgimian commented Sep 24, 2024

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). 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests