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

FT-463 : Integrate golangci-lint for Linting #80

Merged
merged 12 commits into from
Jan 24, 2025
Merged

FT-463 : Integrate golangci-lint for Linting #80

merged 12 commits into from
Jan 24, 2025

Conversation

0xquark
Copy link
Collaborator

@0xquark 0xquark commented Jan 23, 2025

Description

This PR integrates golangci-lint for linting Go code, using tools like govet, gosimple, and staticcheck etc. (see config/.golangci.yaml). It also sets up a GitHub Actions workflow to run the linter on code pushes and pull requests.

Related Issue

FT-463

Checklist

  • My code follows the code style of this project
  • I have added tests to cover my changes (if applicable)
  • I have updated documentation (if applicable)
  • All the checks and tests are passing locally
  • I have verified that the changes works as expected

Further Comments

Signed-off-by: Karanjot Singh <[email protected]>
Signed-off-by: Karanjot Singh <[email protected]>
Signed-off-by: Karanjot Singh <[email protected]>
Signed-off-by: Karanjot Singh <[email protected]>
Signed-off-by: Karanjot Singh <[email protected]>
Signed-off-by: Karanjot Singh <[email protected]>
Signed-off-by: Karanjot Singh <[email protected]>
Signed-off-by: Karanjot Singh <[email protected]>
@0xquark 0xquark added the QOL label Jan 23, 2025
@0xquark 0xquark self-assigned this Jan 23, 2025
@0xquark 0xquark requested a review from Aryamanz29 January 23, 2025 10:28
Copy link
Member

@Aryamanz29 Aryamanz29 left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Just a few questions:

  1. From looking into the config, it seems like it handles a lot of tasks, such as formatting, fixing imports, static checks, etc. I want to confirm: when someone pushes code, does it automatically fix those issues and create a new commit OR does it just highlight specific lines with issues through review comment?
  2. I think we should also document how to run this linter locally in the README.md.
  3. If we can run this locally, we should also integrate it into pre-commit to save CI time (other enhancement req). This way, the linter violations are caught during the pre-commit step before they make it to the CI pipeline ✅

@0xquark
Copy link
Collaborator Author

0xquark commented Jan 23, 2025

  1. From looking into the config, it seems like it handles a lot of tasks, such as formatting, fixing imports, static checks, etc. I want to confirm: when someone pushes code, does it automatically fix those issues and create a new commit OR does it just highlight specific lines with issues through review comment?

When someone runs the linter locally using golangci-lint run, it only shows the errors. To fix them, they need to use golangci-lint run --fix. I’m not adding automatic fixes to the workflow because these decisions should be made by the person after reviewing the linter’s suggestions, as linters can sometimes produce false positives.

  1. I think we should also document how to run this linter locally in the README.md.

I plan to update the README and add the Contributing guidelines, badges etc. in the next PR. I thought it would be better to handle all the documentation updates in one PR to save time and make the review process easier.

  1. If we can run this locally, we should also integrate it into pre-commit to save CI time (other enhancement req). This way, the linter violations are caught during the pre-commit step before they make it to the CI pipeline ✅

Thanks for the suggestion! I forgot about it, but I’ll make sure to include it.

@0xquark 0xquark merged commit c8f68c8 into main Jan 24, 2025
5 checks passed
@0xquark 0xquark deleted the FT-463 branch January 24, 2025 02:47
@0xquark 0xquark mentioned this pull request Jan 30, 2025
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants