Skip to content

Conversation

@dkropachev
Copy link
Contributor

@dkropachev dkropachev commented May 31, 2025

golangci-lint is standard de facto in golang community.
Using will help with:

  • Having code uniformity
  • Enforcing certain linting rules
  • Prevent bugs
  • Prevent unused variables, increasing driver efficiency
  • Easy and speedup reviewing proceccess by detecting problems early

 
govet/fieldalignment stands out from rest of the linters, it will help to define structures in a way that will make them occupy less memory and provide better productivty as result.
 

@dkropachev dkropachev force-pushed the dk/add-linter-golangci branch from 6698573 to c6fb2b2 Compare May 31, 2025 14:56
@dkropachev dkropachev changed the title Switch to golangci instead of govet CASSGO-77: Switch to golangci instead of govet May 31, 2025
@dkropachev dkropachev force-pushed the dk/add-linter-golangci branch 4 times, most recently from 47b1ec5 to 04987d0 Compare June 2, 2025 14:39
@joao-r-reis
Copy link
Contributor

I don't think it's a good idea to say something like "X is standard de facto in golang community" when advocating for a change because a lot of the times when I search online to determine whether this is true or not I'll find a lot of discussion around it and not really any consensus.

In this particular case from what I'm reading it looks like golangci-lint is just a way of running multiple linters so you could just add golanci-lint and run the go vet linter through golanci-lint. Instead what this PR is doing is adding a bunch of more linters instead of just go vet in addition to adding golangci-lint integration.

Personally I'm fine with using golangci-lint but I think the current list of linters that you have is way too large and as a result the diff is huge. This will cause all existing open PRs to have giant conflicts and personally I don't think it's worth the hassle. Also it will probably make it very frustrating to contribute to the driver because how harsh the linter checks are. Maybe you can try running golangci-lint with the default linters only and see if it's a good compromise.

@dkropachev
Copy link
Contributor Author

I don't think it's a good idea to say something like "X is standard de facto in golang community" when advocating for a change because a lot of the times when I search online to determine whether this is true or not I'll find a lot of discussion around it and not really any consensus.

In this particular case from what I'm reading it looks like golangci-lint is just a way of running multiple linters so you could just add golanci-lint and run the go vet linter through golanci-lint. Instead what this PR is doing is adding a bunch of more linters instead of just go vet in addition to adding golangci-lint integration.

Personally I'm fine with using golangci-lint but I think the current list of linters that you have is way too large and as a result the diff is huge. This will cause all existing open PRs to have giant conflicts and personally I don't think it's worth the hassle. Also it will probably make it very frustrating to contribute to the driver because how harsh the linter checks are. Maybe you can try running golangci-lint with the default linters only and see if it's a good compromise.

What we can do here, is to replace govet with golangci with minimal changes, and later on add more linters in a separate PRs, advocating for each of them separately.

@joao-r-reis
Copy link
Contributor

What we can do here, is to replace govet with golangci with minimal changes, and later on add more linters in a separate PRs, advocating for each of them separately.

Yeah that is a good approach 👍

@dkropachev dkropachev force-pushed the dk/add-linter-golangci branch 3 times, most recently from 2986fb8 to e42f913 Compare June 2, 2025 15:33
@dkropachev
Copy link
Contributor Author

What we can do here, is to replace govet with golangci with minimal changes, and later on add more linters in a separate PRs, advocating for each of them separately.

Yeah that is a good approach 👍

Done, please take a look.

@dkropachev dkropachev force-pushed the dk/add-linter-golangci branch from e42f913 to 58f655e Compare June 3, 2025 12:33
Copy link
Contributor

@joao-r-reis joao-r-reis left a comment

Choose a reason for hiding this comment

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

👍 can you squash commits and format the commit message like this? Then I'll merge

1. Switch to use golangci instead of govet
2. Enable all linters in govet, except fieldalingment
3. Run `make fix` to fix all the linting problems
4. Fix awaitSchemaAgreement bug found by linters when error is not
   returned

Patch by dkropachev; reviewed by joao-r-reis for CASSGO-77
@dkropachev dkropachev force-pushed the dk/add-linter-golangci branch from 58f655e to eae72a5 Compare June 3, 2025 12:49
@joao-r-reis joao-r-reis changed the title CASSGO-77: Switch to golangci instead of govet CASSGO-77: Add golangci-lint integration Jun 3, 2025
@joao-r-reis joao-r-reis merged commit 0551773 into apache:trunk Jun 3, 2025
142 of 144 checks passed
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.

2 participants