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

chore: setup golangci-lint #7994

Closed
wants to merge 1 commit into from

Conversation

mmorel-35
Copy link
Contributor

@mmorel-35 mmorel-35 commented Jan 9, 2025

Description

Following #7974 (comment)

Using golangci-lint will provide a common config file for all already used linters.
With this config comments can be provided.

Until now revive was only applied in the main module and not in submodules.
With one tool, this behavior will be avoided.

It will also provide a simpler way to provide new linters

RELEASE NOTES: None

Copy link

codecov bot commented Jan 9, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.12%. Comparing base (aad8a12) to head (252b7f8).
Report is 5 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7994      +/-   ##
==========================================
- Coverage   82.13%   82.12%   -0.01%     
==========================================
  Files         381      381              
  Lines       38542    38542              
==========================================
- Hits        31655    31653       -2     
- Misses       5575     5577       +2     
  Partials     1312     1312              

see 26 files with indirect coverage changes

@mmorel-35 mmorel-35 force-pushed the setup-golangci-lint branch 2 times, most recently from 29de56b to 4d078da Compare January 9, 2025 08:27
@mmorel-35 mmorel-35 marked this pull request as ready for review January 9, 2025 08:30
@mmorel-35 mmorel-35 force-pushed the setup-golangci-lint branch 3 times, most recently from 79a2880 to 1791d04 Compare January 9, 2025 09:28
@mmorel-35
Copy link
Contributor Author

/cc @arjan-bal

@mmorel-35 mmorel-35 force-pushed the setup-golangci-lint branch 12 times, most recently from a2b3351 to 1458678 Compare January 13, 2025 06:39
Signed-off-by: Matthieu MOREL <[email protected]>
@mmorel-35 mmorel-35 force-pushed the setup-golangci-lint branch from 1458678 to 252b7f8 Compare January 13, 2025 06:40
@purnesh42H purnesh42H self-requested a review January 15, 2025 10:18
@purnesh42H purnesh42H self-assigned this Jan 15, 2025
@purnesh42H
Copy link
Contributor

@mmorel-35 thanks for the PR. Overall it looks good with added benefit of checking submodules. I am discussing with other maintainers for any concerns.

@purnesh42H
Copy link
Contributor

@mmorel-35 other maintainers have concerns regarding verifying if all the rules that are replaced are properly running the same checks. There is potentially no way of to verify without adding the problem for each and checking if they are found.

There is definitely a pro that this has integration with more linters than what we're using, so turning on new ones is easy. However, there are bunch of cons as well like it brings in a ton of dependencies, it isn't supported by the maintainer of staticcheck.

Here we are changing a bunch of things for the purpose of making revive run on submodules, but just moving the revive check up a few lines would accomplish that with a lot less churn. Would you be willing to do that instead? Thanks.

@purnesh42H purnesh42H assigned mmorel-35 and unassigned purnesh42H Jan 16, 2025
@purnesh42H purnesh42H added Status: Requires Reporter Clarification Area: Tooling Includes anything related to Go builds, modules etc and includes Releases & Github Workflows. labels Jan 16, 2025
@mmorel-35 mmorel-35 closed this Jan 16, 2025
@mmorel-35 mmorel-35 deleted the setup-golangci-lint branch January 16, 2025 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: Tooling Includes anything related to Go builds, modules etc and includes Releases & Github Workflows. Status: Requires Reporter Clarification
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants