gofumpt icon indicating copy to clipboard operation
gofumpt copied to clipboard

break long lines when it's appropriate to do so

Open jokeyrhyme opened this issue 5 years ago • 59 comments

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

jokeyrhyme avatar Mar 31 '19 06:03 jokeyrhyme

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 avatar Mar 31 '19 14:03 mvdan

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 avatar Mar 31 '19 14:03 mvdan

https://github.com/golang/go/issues/11915 outlines more or less my concerns above. I see this project as a way to experiment with these features.

https://github.com/golang/go/issues/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 avatar Mar 31 '19 15:03 mvdan

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

jokeyrhyme avatar Mar 31 '19 20:03 jokeyrhyme

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

andrewrynhard avatar Aug 17 '19 19:08 andrewrynhard

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.

mvdan avatar Aug 17 '19 20:08 mvdan

Yes, please no options.

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

omeid avatar Aug 25 '19 05:08 omeid

+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?

geoah avatar Oct 01 '19 13:10 geoah

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.

mvdan avatar Oct 02 '19 07:10 mvdan

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

geoah avatar Oct 02 '19 09:10 geoah

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.

Southclaws avatar Oct 17 '19 14:10 Southclaws

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.

gnu-user avatar Nov 11 '19 17:11 gnu-user

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.

mvdan avatar Nov 11 '19 17:11 mvdan

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

kovetskiy avatar Dec 06 '19 13:12 kovetskiy

Anyone working on this?

remorses avatar May 08 '20 15:05 remorses

@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 avatar May 08 '20 16:05 mvdan

I put together a rough first cut of this feature in https://github.com/mvdan/gofumpt/pull/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.

mvdan avatar May 12 '20 14:05 mvdan

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)

geoah avatar May 12 '20 15:05 geoah

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

andrewrynhard avatar May 12 '20 15:05 andrewrynhard

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.

mvdan avatar May 12 '20 16:05 mvdan

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?

andrewrynhard avatar May 12 '20 16:05 andrewrynhard

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.

rogpeppe avatar May 12 '20 17:05 rogpeppe

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.

andrewrynhard avatar May 12 '20 17:05 andrewrynhard

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.

Southclaws avatar May 12 '20 20:05 Southclaws

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.

mvdan avatar May 12 '20 22:05 mvdan

@mvdan that makes sense thank you for the explanation.


My main concern with the conservative approach is the merging of small lines into things that (imo) are much less readable.

For example;

var (
	AccountRequestsDefaultPermissions = []string{
		"ReadAccountsDetail",
		"ReadBalances",
		"ReadBeneficiariesDetail",
		"ReadDirectDebits",
		"ReadProducts",
		"ReadStandingOrdersDetail",
		"ReadTransactionsCredits",
		"ReadTransactionsDebits",
		"ReadTransactionsDetail",
	}
)

becomes

var AccountRequestsDefaultPermissions = []string{"ReadAccountsDetail", "ReadBalances",
	"ReadBeneficiariesDetail", "ReadDirectDebits", "ReadProducts", "ReadStandingOrdersDetail",
	"ReadTransactionsCredits", "ReadTransactionsDebits", "ReadTransactionsDetail"}

which btw if you re-gofumpt then becomes

var AccountRequestsDefaultPermissions = []string{
	"ReadAccountsDetail", "ReadBalances",
	"ReadBeneficiariesDetail", "ReadDirectDebits", "ReadProducts", "ReadStandingOrdersDetail",
	"ReadTransactionsCredits", "ReadTransactionsDebits", "ReadTransactionsDetail",
}

geoah avatar May 13 '20 09:05 geoah

@geoah not sure if I understand - if we number your three snippets as 1-2-3, the WIP branch should never convert 1 into 2. It should only add newlines, not remove them.

The fact that you need to re-run gofumpt to turn 2 into 3 is a bug, which was already fixed in the latest commit I think; see https://github.com/mvdan/gofumpt/pull/70#issuecomment-627640487.

mvdan avatar May 13 '20 10:05 mvdan

@mvdan erm. o_O

Do you mind running this test? https://github.com/mvdan/gofumpt/compare/break-long-lines...geoah:break-long-lines

I get a failure with

        -var AccountRequestsDefaultPermissionsMessy = []string{
        -       "ReadAccountsDetail", "ReadBalances",
        -       "ReadBeneficiariesDetail", "ReadDirectDebits", "ReadProducts", "ReadStandingOrdersDetail",
        -       "ReadTransactionsCredits", "ReadTransactionsDebits", "ReadTransactionsDetail",
        -}
        +var (
        +       AccountRequestsDefaultPermissionsMessy = []string{
        +               "ReadAccountsDetail",
        +               "ReadBalances",
        +               "ReadBeneficiariesDetail",
        +               "ReadDirectDebits",
        +               "ReadProducts",
        +               "ReadStandingOrdersDetail",
        +               "ReadTransactionsCredits",
        +               "ReadTransactionsDebits",
        +               "ReadTransactionsDetail",
        +       }
        +)

The weird thing is that this happens only when there is no comment above the var block.

Also this doesn't happen if there is no var block, but the var is inlined.


At this point I'll have to apologise in advance as I'm probably doing something weird and/or wrong. :P

geoah avatar May 13 '20 10:05 geoah

asdf. - ^ this also exists in the master gofumpt. sorry for this. :( in master

var (
	AccountRequestsDefaultPermissions = []string{
		"ReadAccountsDetail",
		"ReadBalances",
		"ReadBeneficiariesDetail",
		"ReadDirectDebits",
		"ReadProducts",
		"ReadStandingOrdersDetail",
		"ReadTransactionsCredits",
		"ReadTransactionsDebits",
		"ReadTransactionsDetail",
	}
)

Becomes

var AccountRequestsDefaultPermissions = []string{"ReadAccountsDetail", "ReadBalances", "ReadBeneficiariesDetail", "ReadDirectDebits", "ReadProducts", "ReadStandingOrdersDetail", "ReadTransactionsCredits", "ReadTransactionsDebits", "ReadTransactionsDetail"}

I just didn't notice this before I guess.

geoah avatar May 13 '20 10:05 geoah

Yes, that definitely sounds like a bug in master. Please file a separate issue, and I'll hide these comments since they're off-topic for this feature/PR.

mvdan avatar May 13 '20 10:05 mvdan

From Effective Go: "Go has no line length limit. Don't worry about overflowing a punched card. If a line feels too long, wrap it and indent with an extra tab." https://golang.org/doc/effective_go.html#formatting

Just my two cents, I don't think this should be part of gofumpt. Formatters should be there for when there is an obvious, idiomatic modification that can be made to source code, not for style preference.

Also not sure if this is the time or place but don't understand why something like this would not be configurable, especially as part of something like golangci-lint. For example, if I love all the adjustments made by gofumpt except one (like this one), I have to stop using the tool entirely.

kylechadha avatar Jul 23 '20 01:07 kylechadha

If a line feels too long, wrap it and indent with an extra tab.

This is very subjective, and can be labor intensive to do consistently, and also a time-waster in code reviews, which is part of the value of an opinionated line-break tool

don't understand why something like this would not be configurable

This is a delicate balancing act that the prettier folks have struggled with as well

Options tend to increase adoption, but also increase code complexity, so it's sort of up to each community / author to decide whether adoption is more important than simplicity / reliability

jokeyrhyme avatar Jul 23 '20 01:07 jokeyrhyme

I agree with @jokeyrhyme, basically.

@kylechadha I also used to think that line length shouldn't matter. And I definitely agree that a hard limit on line length is a terrible idea. Somewhere in between, humans make a judgement call, and I'm trying to encode that into a heuristic. I think the current state of the PR is already a pretty good approximation, but I would need to finish it up and furiously test it on large amounts of "idiomatic" Go code.

As for the configurability - you quote the Go docs, so I should mention that one big design decision from gofmt is to not make formatting configurable. Once you do that, different projects will use different, incompatible styles. I definitely want to follow the same rule of thumb here, so this will not be configurable. If it's annoying or incorrect, then we should fix or remove the feature entirely, not expect the user to turn it off.

mvdan avatar Jul 28 '20 15:07 mvdan

one big design decision from gofmt is to not make formatting configurable. Once you do that, different projects will use different, incompatible styles

@mvdan If that's your goal then I find it interesting you're creating a second formatter to begin with. You've done exactly what your statement above claims to avoid: different projects with different styles. Some formatted only with gofmt, some formatted with both gofmt / gofumpt.

Don't really have a bone to pick in this issue so of course do whatever makes sense for your project, but if we want to have an opinionated, non standard formatter, it would make sense to me to allow it to be opinionated based on anyone's opinion. After all, in the case of line length, you're going against the Go authors' opinion of what idiomatic is. Ie: there is no single source of truth here any way.

@jokeyrhyme The point of not having an automated line-break tool is because what you gain (not having to think about line length) is less valuable than what you lose (lines broken intelligently rather than arbitrarily). If the goal of consistent styles is to decrease cognitive complexity, I would argue this harms that.

kylechadha avatar Jul 30 '20 04:07 kylechadha

Some formatted only with gofmt, some formatted with both gofmt / gofumpt.

I think there's an effort to keep the output of gofumpt compatible with gofmt, that is: gofmt will take gofumpt-formatted code and not detect anything it needs to change

It's in the README.md:

Enforce a stricter format than gofmt, while being backwards compatible. That is, gofumpt is happy with a subset of the formats that gofmt is happy with.

jokeyrhyme avatar Jul 30 '20 05:07 jokeyrhyme

With Prettier I can add a // at end of first line and it'll always split the rest on individual lines, because code cannot follow the comment marker. Prettier heuristic isn't perfect, so it's nice to have the option to force it. Will this support a similar one? Or dump all of the rest on one line still?

samlof avatar Jul 30 '20 17:07 samlof

You've done exactly what your statement above claims to avoid: different projects with different styles.

That's true in a way, but it's impossible to avoid this separation. I can't take over gofmt or attempt to replace it entirely. If anyone wants to experiment with a stricter gofmt (but backwards compatible, like @jokeyrhyme said), it has to be a separate project.

So I can't get rid of the gofmt vs gofumpt separation. I can, however, prevent more dimensions to be added in the form of extra options.

in the case of line length, you're going against the Go authors' opinion of what idiomatic is

I already touched on this topic, but to repeat my thinking - their stance is generally that line length limits are silly, I agree. They do not say that line length does not matter. If that were the case, most idiomatic Go code wouldn't typically fit within 80-120 columns or so. Significantly longer lines are definitely not considered idiomatic in general.

Again, there's noone forcing you to use gofumpt. If gofmt is enough for you, that's absolutely fine. I'm always looking for more input to fine-tune gofumpt to be as good as possible, but the lack of options and the backwards compatibility are pretty much non-negotiable. The moment this tool loses either, it would stop being useful as a companion to gofmt.'

@samlof assuming you're talking about preventing prettier from joining lines, I don't think you should worry about that, because gofumpt won't join short lines into regular-sized ones. This issue is only about splitting very long lines as long as they are easy to split into more reasonable lines.

mvdan avatar Aug 02 '20 13:08 mvdan

As a semi-random thought, it would be convenient to have something that can pretty-format the single-line output of %#v in fmt.Sprintf("package foo\nvar data = %#v\n", superBigSliceOfMaps).

quasilyte avatar Aug 20 '20 16:08 quasilyte

The irony is that Effective Go is formatted with a consistent line length, presumably for readability reasons.

It's not just about punch cards - typographers have been thinking about this stuff for the past 100 years or so.

patspam avatar Nov 08 '20 06:11 patspam

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!

mvdan avatar Mar 11 '21 16:03 mvdan

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

StevenACoffman avatar Mar 25 '21 17:03 StevenACoffman

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

csilvers avatar Mar 25 '21 21:03 csilvers

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

mvdan avatar Apr 05 '21 11:04 mvdan

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.

mvdan avatar Apr 29 '21 09:04 mvdan

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

jokeyrhyme avatar Apr 29 '21 10:04 jokeyrhyme

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.

mvdan avatar Apr 29 '21 10:04 mvdan

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;
    }

jokeyrhyme avatar Apr 29 '21 20:04 jokeyrhyme

If anyone else is looking for a decent solution, golines is an excellent option.

gideongrinberg avatar May 30 '21 01:05 gideongrinberg

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.

Seirdy avatar Jan 23 '22 22:01 Seirdy

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 avatar May 14 '22 19:05 1Mark

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

1Mark avatar May 14 '22 19:05 1Mark