Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

feat: add linter pairedbrackets#3225

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Closed
maratori wants to merge1 commit intogolangci:masterfrommaratori:pairedbrackets
Closed

feat: add linter pairedbrackets#3225

maratori wants to merge1 commit intogolangci:masterfrommaratori:pairedbrackets

Conversation

@maratori
Copy link
Contributor

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

@ldez
Copy link
Member

This kind of linter already exists inside golangci-lint.

@maratori
Copy link
ContributorAuthor

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

@ldez
Copy link
Member

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

@maratori
Copy link
ContributorAuthor

maratori commentedSep 17, 2022
edited
Loading

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

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

BadGood
fmt.Printf("%s, %s!","Hello","world")
fmt.Printf("%s, %s!","Hello","world",)
fmt.Printf("%s, %s!","Hello","world")
fmt.Printf("%s %s","Last",`itemis multiline`,)
fmt.Printf("%s, %s!","Hello","world")
fmt.Printf("%s, %s!","Hello","world",)
fmt.Printf("%s, %s!","Hello","world",)
fmt.Printf("%s %s","Last",`itemis multiline`)

@ldez
Copy link
Member

ldez commentedSep 17, 2022
edited
Loading

🤔

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.

cristaloleg reacted with thumbs up emoji

@maratori
Copy link
ContributorAuthor

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
Copy link
Member

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
Copy link
Member

Just to be clear, I'm just chatting.

@ldezldez added the linter: newSupport new linter labelSep 17, 2022
@ldezldez self-requested a reviewSeptember 17, 2022 21:32
@ldezldez added the waiting for: contributor feedbackRequires additional feedback labelSep 17, 2022
@ldez
Copy link
Member

@bombsimon what is your opinion?

@maratori
Copy link
ContributorAuthor

maratori commentedSep 17, 2022
edited
Loading

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, seeconfigignore-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 otherexamples? Like function declaration, or composite literals.
Are they also too strict?

@ldez
Copy link
Member

ldez commentedSep 17, 2022
edited
Loading

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
Copy link
Member

ldez commentedSep 17, 2022
edited
Loading

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 thestrings.NewReplacer() or CLI args).

@maratori
Copy link
ContributorAuthor

I didn't get your point about composite literals.

Following is a good format according topairedbrackets:

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.

ldez reacted with thumbs up emoji

@bombsimon
Copy link
Member

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 ingolanci-lint.

I agreeignore-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)

Regardingenable-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 theenable-all setting. At least that's what I do.

@ldez
Copy link
Member

ldez commentedSep 17, 2022
edited
Loading

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
Copy link
Member

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

funcFoo(aint,bstring,cbool) {...}funcFoo() (int,error) {...}foo:= []int{1,2,3}
ldez reacted with thumbs up emoji

@maratorimaratori closed this by deleting the head repositoryNov 10, 2022
@maratorimaratori reopened thisNov 10, 2022
@maratori
Copy link
ContributorAuthor

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
Copy link
Member

ldez commentedDec 25, 2022
edited
Loading

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

@Antonboom
Copy link
Contributor

Antonboom commentedOct 1, 2023
edited
Loading

@maratori
What are the next steps here?

@ldez
Copy link
Member

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.

@ldezldez closed thisMar 18, 2024
@bombsimonbombsimon mentioned this pull requestJul 1, 2024
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@ldezldezAwaiting requested review from ldez

Assignees

No one assigned

Labels

linter: newSupport new linterwaiting for: contributor feedbackRequires additional feedback

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@maratori@ldez@bombsimon@Antonboom

[8]ページ先頭

©2009-2025 Movatter.jp