feat: add linter pairedbrackets

[maratori]

pairedbrackets is a linter that checks formatting of paired brackets.
https://github.com/maratori/pairedbrackets

[ldez]

This kind of linter already exists inside golangci-lint.

[maratori]

What linter does the same? I haven't seen it.

[ldez]

At least: gofumpt
from my memories, there is another one.

[maratori]

As I noted in readme, gofumpt doesn't cover all cases.

For example it isn't complain about following (all of them are good for gofumpt)

Bad	Good
fmt.Printf("%s, %s!", "Hello", "world") fmt.Printf("%s, %s!", "Hello", "world", ) fmt.Printf( "%s, %s!", "Hello", "world") fmt.Printf("%s %s", "Last", `item is multiline`, )	fmt.Printf("%s, %s!", "Hello", "world") fmt.Printf( "%s, %s!", "Hello", "world", ) fmt.Printf( "%s, %s!", "Hello", "world", ) fmt.Printf("%s %s", "Last", `item is multiline`)

[ldez]

🤔

Your example:

fmt.Printf("%s, %s!",
	"Hello", "world")

is not bad from my point of view, it's common when the pattern is long or to improve readability.

I evaluate the other example as more bad than good:

fmt.Printf(
	"%s, %s!", "Hello", "world",
)

Otherwise, I'm checking the other linters.

[maratori]

is not bad from my point of view

I understand you, and I'm sure you are not alone.
But I'm not alone either. Before implementation, I've validated an idea with colleagues.

There are a lot of opinionated linters in golangci-lint. pairedbrackets is one of them.
That's why most of the linters are disabled by default.

[ldez]

There are a lot of opinionated linters in golangci-lint. pairedbrackets is one of them.
That's why most of the linters are disabled by default.

yes there are opinionated linters but here I see more false-positive than opinion.

A lot of people (I think the majority) enable all linters and try to follow them.

So the default behavior of linter must be something that we can recommend.

[ldez]

Just to be clear, I'm just chatting.

[ldez]

@bombsimon what is your opinion?

[maratori]

Just to be clear, I'm just chatting

👌

I see more false-positive than opinion

I've run the linter on our code base and found false positives only in tests: assert.Equal(...), require.Equal(...) and so on.
All of them are ignored by default, see config ignore-func-calls.
Other issues are not false positives from my point of view.

A lot of people (I think the majority) enable all linters and try to follow them.
So the default behavior of linter must be something that we can recommend.

I can add a config to ignore all function calls by default.
What do you think about other examples? Like function declaration, or composite literals.
Are they also too strict?

[ldez]

I think the "ignore function calls" is not a good option, because I don't want to ignore rules with some functions, I just want to ignore some recommendations.
For example, I can agree with the previous examples 3 and 4 but not on 1 and 2.

Also, I don't understand the default exclusion of the testify functions.

[ldez]

There are also some problems with the other elements, for example with Composite literal: if I want to have a slice with values organized by pair (like with the strings.NewReplacer() or CLI args).

[maratori]

I didn't get your point about composite literals.

Following is a good format according to pairedbrackets:

strings.NewReplacer([]string{
	"a", "A",
	"b", "B",
}...)

It doesn't expect items to be formatted in one column. It just checks opening and closing brackets.

[bombsimon]

I think this linter makes sense and as someone that in general appreciate consistent styling I would probably even use this myself (although that doesn't really matter). I think the reasoning around the linter, the link to the article and the clear examples is more than enough to argue for its use case. It's absolutely opinionated but like you both stated that goes for a lot of the linters in golanci-lint.

I agree ignore-func-calls is weird, I don't see why this shouldn't apply for all functions. I would rather see option(s) to allow some styles although I fully understand that's more work. Curious about the false positives, what were they?

I think I agree with all cases the linter enforces except the one that @ldez pointed out. To me, this is "balanced" and paired (left parenthesis and first argument on same line, last argument and parenthesis on same line). Maybe an option to allow this?

fmt.Sprintf("This is too long for one line but ok for two %s: %d",
    someString, someInt)

Regarding enable-all; I actually don't think a majority use it. And for those who does, I think it's more common to discover new linters but if they're not relevant for the user they'll be disabled while still using the enable-all setting. At least that's what I do.

[ldez]

Regarding enable-all; I actually don't think a majority use it.

I think we need data about that because we are just expressing opinions 😸.
Maybe at some point, it can be a good idea to try to get information about the usage of golangci-lint.
But it's a bit off-topic.

And for those who does, I think it's more common to discover new linters but if they're not relevant for the user they'll be disabled while still using the enable-all setting.

I do that too, but I have seen different behavior with the handle of a new linter:

• if a linter doesn't have the "right" default behavior they just ignore it.
• some people just follow the rules because "linters say the truth".

This is why for me the default behavior is really important.

[bombsimon]

I think we need data about that because we are just expressing opinions 😸.

You are right! 👍

This is why for me the default behavior is really important.

Makes sense and I agree. With that in mind maybe a default "relaxed" mode would be nice with the option to be more strict if desired.

From my point of view the only needed change would be to allow the above example by default and optionally disallow it for those who don't agree and care to read the docs. All the others, including the ones in the readme, makes sense and is easy to understand why they're there in my opinion.

I guess such a relax mode would do the same for functions and composite literals as well although that makes way less sense to me personally. 😄

func Foo(a int,
	b string, c bool) {
	...
}

func Foo() (int,
	error) {
	...
}

foo := []int{1,
	2, 3}

[maratori]

Google published their style guide recently. Looks like they use "relaxed" version of linter's rules (link1, link2):

The closing half of a brace pair should always appear on a line with the same amount of indentation as the opening brace.

Will you accept the linter to be a part of golangci-lint, if I change it?

1. By default the linter will follow the quote above
2. ignore-func-calls option will be removed
3. strict option will be added to follow the original rules

[ldez]

Yes if your linter handles the cases we talked about.
I am unsure about the term strict or the option itself but we will see during the review.

[Antonboom]

@maratori
What are the next steps here?

[ldez]

I will close this PR because:

• No feedback for a long time from the PR author.
• The code of golangci-lint has changed since 2022.

Feel free to open a new PR, when the linter and the implementation inside golangci-lint will be updated.