8000 ci: run golangci-lint by klaernie · Pull Request #437 · editorconfig-checker/editorconfig-checker · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

ci: run golangci-lint #437

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

ci: run golangci-lint #437

wants to merge 16 commits into from

Conversation

klaernie
Copy link
Member
@klaernie klaernie commented Feb 2, 2025

Since @ccoVeille mentioned it in #434, I thought we should give it a spin.

@klaernie klaernie force-pushed the golangci-lint branch 2 times, most recently from 3ab307c to 5a2121f Compare February 2, 2025 21:32
@klaernie
Copy link
Member Author
klaernie commented Feb 2, 2025

Hm, with the current configuration we are at 334 found issues - now I wonder if we should just accept the current issues and just disallow new ones, or refine the list of enabled linters. Quite a few ones are definitely helpful - we have for example many cases where we create new errors on the fly, which would be better solved by defining constants.

Do you have any cool default set to use, @ccoVeille ?

@ccoVeille
Copy link
Contributor
ccoVeille commented Feb 2, 2025 •
edited
Loading

I do !

https://github.com/ccoVeille/golangci-lint-config-examples

Please also note, the settings you chose might seem pretty complete, while it isn't. You cannot imagine, trust me.

linters: 
   enable-all: true 
   disable: 
     # all currently deprecated linters 
     - execinquery 
     - exportloopref 
     - gomnd

And it's not your fault.

Many golangci-lint linters are using default rulesets that all pretty lax, and not strict.

There are a lot of things to enable in govet, gocritict, staticcheck, revive to get interesting settings. Please note, not all of them are interesting. Some are in forcing rules barely no one want. Such as "never reuses a variable for error, define new one".

Discovering, enabling, and configuring linters is a journey, as interesting and perilous than going to the Mordor. You can lose yourself.

That's why I created my project of examples of golangci-lint configuration examples.

BTW, I think I will have to update my README file with something close to what I just wrote.

EDIT: I opened

@ccoVeille
Copy link
Contributor
ccoVeille commented Feb 2, 2025 •
edited
Loading

Then to reply your questions about what could be the path to enable and fix the issues.

I would say the strategy depends on the time you want to spend on fixing the issues golangci-lint may report.

I would say it's important to also do not block contributions to project.

There are many ways to achieve this.

Define a .golangci.yml pretty strict at the project root. This way IDE will report the issues. So core developers and advanced Go contributors who know golangci-lint will see what they need to fix.

Then you have multiple options:

  • you define a relaxed golangci-lint configuration option for the CI, that can stay in .github folder.

So here, you have somehow to fix the issues reported by this configuration files before enforcing the CI to block them. Otherwise, you block contributions.

  • second option is to play with golangci-lint run -new that can be used locally, but also configured on the golangci-lint-action via the with field.

This feature is there to say "OK, old code sucks, we know that, but we want new code to be clean"

Both option can be used at same time, or separately.

Everything is a matter of balance:

  • the quality you want for your code.
  • the amount of linters you want to use
  • the amount of issues currently in the code base
  • the amount of time you want to spend on fixing first batch of errors reported with a relaxed set of rules
  • how much people will understand/agree the errors reported are qualitative and not noise, opinionated.

It seems to be complex, but the gain is real. Also, don't be afraid, it's a journey, time is needed, but there are multiple rewards in trying such a quest:

  • working with golangci-lint rules was a huge opportunity for me. It learned a lot of valuable things in Go, but also as a developer/architect/team lead.
  • I know a lot of these rules now. I write code that complies them by default now.
  • the number of bugs I caught with the CI before they were merged is insane. People with less experience in Go, development skills, and unfortunately people who don't care about shipping "bad code that works", they will all face the same CI warning and blockers.
  • the gain is important for code reviewers as the "obvious" errors are reported. So when you receive a PR, most little errors were caught and fixed before you review
  • the gain is huge as a maintainer, it catches real bug that experienced reviewers can miss.

I talked a lot, I hope I didn't afraid you. It's a very interesting and fascinating topic.

I can help of course, even if I'm less active right now for personal reason.

@ccoVeille ccoVeille mentioned this pull request Feb 2, 2025
Open
@klaernie
Copy link
Member Author
klaernie commented Feb 4, 2025

I was hoping for such a good base configuration to get started - and 17 issues is much more manageable that over 300.

Fixing the first set of issues was quite easy, but now I'm a bit stuck on how to handle the remaining three:

kandre@mainframe(pts/18)[golangci-lint!] ~/git-repos/ecc/editorconfig-checker % golangci-lint run
pkg/validation/validators/validators.go:14:1: File is not properly formatted (gci)
        // x-release-please-end
^
pkg/validation/validators/validators_test.go:10:1: File is not properly formatted (gci)
        // x-release-please-end
^
pkg/config/config.go:16:1: File is not properly formatted (gci)
        // x-release-please-end
^
kandre@mainframe(pts/18)[golangci-lint] ~/git-repos/ecc/editorconfig-checker %

klaernie added 16 commits March 6, 2025 23:13
@klaernie
ci: run golangci-lint
401ad8e
@klaernie
fix(lint): errors should be printed as strings
6abb84c 
Since this was done with the go1.24 migration differently, I just
remembered that printing the errors as strings should be the simpler
approach.
@klaernie
fix(lint): declare that we intentionally ignore this returned error
ca1b230
@klaernie
ci: enable all golangci-lint linters by default
3ec921d
@klaernie
ci: disable the deprecated linters
d848e2d
@klaernie
ci: use a more sensible starting point for golangci-lint
b29449f
@klaernie
ci: ignore a misspelling, since it is a username, not an actual word
bf96257
@klaernie
fix(lint): keep name consistent with the other functions
714107c
@klaernie
fix(lint): rename variable to camelCase
ba0e539
@klaernie
fix(lint): discovered more typos
3f08cd1
@klaernie
fix(lint): arguments of type testing.T should always be first
1f035f3
@klaernie
fix(lint): listAllCharsets() is a helper function
e6262fe
@klaernie
fix(lint): func GetContentTypeBytes() doesn't need the config, so rem…
0a3644b 
…ove the arg

then remove the now unused variables and arguments from the functions
earlier in the callchain
@klaernie
fix(lint): remove initialization to the zero value
9d93d5d
@klaernie
fix(lint): infer types
c37482a
@klaernie
ci: switch from gci to goimports
98b6d2b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@klaernie @ccoVeille
0