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

Adding gofumpt #1177

Merged
merged 9 commits into from Jun 16, 2020
Merged

Adding gofumpt #1177

merged 9 commits into from Jun 16, 2020

Conversation

teivah
Copy link
Member

@teivah teivah commented Jun 5, 2020

Hey!
I was also interested in #490 (adding gofumpt).
Here is a PR for it.

@boring-cyborg
Copy link

boring-cyborg bot commented Jun 5, 2020

Hey, thank you for opening your first Pull Request !

@CLAassistant
Copy link

CLAassistant commented Jun 5, 2020

CLA assistant check
All committers have signed the CLA.

@teivah teivah mentioned this pull request Jun 5, 2020
issues = append(issues, goanalysis.NewIssue(&result.Issue{
FromLinter: gofumptName,
Text: "File is not `gofumpt`-ed",
Pos: token.Position{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a Replacement parameter to automatically fix issues, which is very useful for gofumpt.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm not mistaken, gofumpt provides only as a public function a way to get the gofumpt-ed version of the file (as a []byte).
With Replacement you mean I should include the gofumpt-ed version of the whole file in the issue?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@tetafro Am I correct?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gofumpt provides only as a public function a way to get the gofumpt-ed version of the file

Yeah, sorry, I didn't think of it.

With Replacement you mean I should include the gofumpt-ed version of the whole file in the issue?

I'm not sure if it's a right thing to do. Someone more experienced with golangci-lint architecture should answer.


Anyway, if there's no easy way to do this, just skip my first comment.

@tetafro
Copy link

tetafro commented Jun 9, 2020

Can you please add test to tests/testdata?

issues = append(issues, goanalysis.NewIssue(&result.Issue{
FromLinter: gofumptName,
Text: "File is not `gofumpt`-ed",
Pos: token.Position{
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gofumpt provides only as a public function a way to get the gofumpt-ed version of the file

Yeah, sorry, I didn't think of it.

With Replacement you mean I should include the gofumpt-ed version of the whole file in the issue?

I'm not sure if it's a right thing to do. Someone more experienced with golangci-lint architecture should answer.


Anyway, if there's no easy way to do this, just skip my first comment.

@@ -191,6 +191,10 @@ func (m Manager) GetAllSupportedLinterConfigs() []*linter.Config {
WithPresets(linter.PresetFormatting).
WithAutoFix().
WithURL("https://golang.org/cmd/gofmt/"),
linter.NewConfig(golinters.NewGofumpt()).
WithPresets(linter.PresetFormatting).
WithAutoFix().
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If there is no autofix, you should remove this line

@teivah
Copy link
Member Author

teivah commented Jun 9, 2020

Can you please add test to tests/testdata?

@tetafro I've just added a test. Just something to mention, previously I was setting the error line to 0. I wanted to change it to line 1 but in the test file, I can't do this:

// ERROR "File is not `gofumpt`-ed"
//args: -Egofumpt
package testdata

Hence, I set it to the last line number and it's working correctly.

@teivah teivah requested a review from tetafro June 9, 2020 08:19
@tetafro
Copy link

tetafro commented Jun 12, 2020

Hence, I set it to the last line number and it's working correctly.

I've looked how gofmt linter is implemented. Seems like the right thing to do here is to re-use diff code from gofmt linter.

But I think using the last line is ok for the first iteration. Maybe just add TODO in the code.

return nil, fmt.Errorf("error while running gofumpt: %w", err)
}
if !bytes.Equal(input, output) {
reader, err := os.Open(f)
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it really necessary? You've already read the file into input. Just use strings.Count.

@teivah teivah requested a review from tetafro June 12, 2020 19:47
Copy link

@tetafro tetafro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! This looks good to me.

@teivah
Copy link
Member Author

teivah commented Jun 15, 2020

Thanks! This looks good to me.

@tetafro Any chance to get it merged then? :)

@tetafro
Copy link

tetafro commented Jun 16, 2020

Any chance to get it merged then? :)

Yeah, sorry for delay

@tetafro tetafro merged commit 38d298c into golangci:master Jun 16, 2020
@golangci-automator
Copy link

Hey, @teivah — we just merged your PR to golangci-lint! 🔥🚀

golangci-lint is built by awesome people like you. Let us say “thanks”: we just invited you to join the GolangCI organization on GitHub.
This will add you to our team of maintainers. Accept the invite by visiting this link.

By joining the team, you’ll be able to label issues, review pull requests, and merge approved pull requests.
More information about contributing is here.

Thanks again!

@teivah
Copy link
Member Author

teivah commented Jun 16, 2020

Thanks @tetafro

@gonzaloserrano
Copy link

Hi, how's the doc updated once a new linter is merged?

@teivah
Copy link
Member Author

teivah commented Jun 22, 2020

@tetafro Just out of curiosity, when do you plan roughly to make a new release?
Thanks

@teivah teivah deleted the gofumpt branch June 22, 2020 07:47
@tetafro
Copy link

tetafro commented Jun 22, 2020

@teivah this is a question for guys from core team. I just help with pr reviews from time to time.

@SVilgelm SVilgelm linked an issue Jul 13, 2020 that may be closed by this pull request
@ldez ldez added the linter: new Support new linter label Dec 7, 2020
@ldez ldez added this to the v1.28 milestone Mar 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
linter: new Support new linter
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add gofumpt
5 participants