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

CI: Add gofmt check in lint #263

Closed
wants to merge 2 commits into from
Closed

Conversation

singh1203
Copy link
Contributor

@singh1203 singh1203 commented Jan 29, 2025

Fixes: #58

What changes are included in this PR?

Added the gofmt check in the pre-commit-config.yml, which identifies files that need gofmt linting and make proper linting.

Are these changes tested?

Yes, I tested it on my forked repository example here

Signed-off-by: Saurabh Kumar Singh <singh1203.ss@gmail.com>
@zeroshade
Copy link
Member

Would it make more sense to include this in the golangci configuration that we use with pre-commit instead? Particularly if we could have it so that pre-commit runs gofmt for the user too

@singh1203
Copy link
Contributor Author

Would it make more sense to include this in the golangci configuration that we use with pre-commit instead? Particularly if we could have it so that pre-commit runs gofmt for the user too

Yes, you are correct. I raised the same question in yesterday's Arrow community meeting. I will incorporate the changes as per the review.

@singh1203
Copy link
Contributor Author

Would it make more sense to include this in the golangci configuration that we use with pre-commit instead? Particularly if we could have it so that pre-commit runs gofmt for the user too

@zeroshade I made changes by adding a hook, which you can see here. This raises the question: do I need to commit the changed files after running pre-commit cmd locally?

@zeroshade
Copy link
Member

@zeroshade I made changes by adding a hook, which you can see here. This raises the question: do I need to commit the changed files after running pre-commit cmd locally?

That's the way it would work, yes. I'm not sure if we'd want it to automatically commit the changed files.

Signed-off-by: Saurabh Kumar Singh <singh1203.ss@gmail.com>
@singh1203
Copy link
Contributor Author

singh1203 commented Feb 5, 2025

@zeroshade I made changes by adding a hook, which you can see here. This raises the question: do I need to commit the changed files after running pre-commit cmd locally?

That's the way it would work, yes. I'm not sure if we'd want it to automatically commit the changed files.

Yes you were right I need to commit, and also, instead of hook, I chose to go ahead with go fmt cmd since that pre-commit hook is no more being maintained. Aside from that, I think we can run the pre-commit run before pushing rather than autocommitting.
@kou Any thoughts for this?

@kou
Copy link
Member

kou commented Feb 6, 2025

Can we use https://github.com/golangci/golangci-lint ?
It seems that it includes gofmt support: https://golangci-lint.run/usage/configuration/

We don't need to commit changes by pre-commit automatically. Developers should do it after they checked.

@singh1203
Copy link
Contributor Author

singh1203 commented Feb 6, 2025

Can we use https://github.com/golangci/golangci-lint ? It seems that it includes gofmt support: https://golangci-lint.run/usage/configuration/

We don't need to commit changes by pre-commit automatically. Developers should do it after they checked.

Yes, we have integrated https://github.com/golangci/golangci-lint in our pre-commit-config.yaml and have explicitly configured the gofmt linter. However, linting does not appear to be taking effect. Running gofmt manually produces the expected formatting results. Additionally, if gofumpt is used as the linter, the output differs from gofmt, indicating that it enforces formatting rules that deviate from the standard gofmt behaviour.

@singh1203
Copy link
Contributor Author

Can we use https://github.com/golangci/golangci-lint ? It seems that it includes gofmt support: https://golangci-lint.run/usage/configuration/

We don't need to commit changes by pre-commit automatically. Developers should do it after they checked.

Also, I just learned that gofmt is integrated as a linter, not as a formatter, so its scope is limited to the current module in https://github.com/golangci/golangci-lint

@kou
Copy link
Member

kou commented Feb 7, 2025

Oh, I forgot that we already have golangci/golangci-lint configuration!

It seems that it works well. So I think that we don't need to do this.

The changed files in this PR have "DO NOT EDIT" comment because they are auto generated files. So we should not target them.

It seems that golangci/golangci-lint excepts them automatically. And it's an expected behavior.

@singh1203
Copy link
Contributor Author

Oh, I forgot that we already have golangci/golangci-lint configuration!

It seems that it works well. So I think that we don't need to do this.

The changed files in this PR have "DO NOT EDIT" comment because they are auto generated files. So we should not target them.

It seems that golangci/golangci-lint excepts them automatically. And it's an expected behavior.

Okay 👍 Then I should proceed with closing this PR.
cc- @zeroshade

@kou
Copy link
Member

kou commented Feb 7, 2025

Sorry. I should have closed #58 when I did #92...

@kou kou closed this Feb 7, 2025
@singh1203 singh1203 deleted the gofmt-ci branch February 7, 2025 05:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Go][CI]: Run gofmt check in CI
3 participants