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.

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

Update and optimize linting configuration #3179

Closed
4 tasks done
boulder225 opened this issue Jul 22, 2024 · 1 comment
Closed
4 tasks done

Update and optimize linting configuration #3179

boulder225 opened this issue Jul 22, 2024 · 1 comment
Assignees
@KaloyanTanev
Labels
protocol Protocol Team tickets

Comments

@boulder225
Copy link

boulder225 commented Jul 22, 2024 •
edited by KaloyanTanev
Loading

🎯 Problem to be solved

The current linting setup has several issues that need to be addressed:

  1. Many assertions are inside go routines, which may lead to unreliable test results.
  2. The pre-commit hook uses a default golangci-lint configuration, causing inconsistencies between local and pipeline checks.
  3. Linter output is limited, potentially hiding numerous issues.
  4. The golangci-lint action in the pipeline is outdated.

🛠️ Proposed solution

  • Replace assertions inside go routines with a testing method (addressed by testifylint)
  • Update pre-commit hook to use golangci-lint with the same version as in pipelines
  • Remove limits on linter output by setting max-same-issues=0 and max-issues-per-linter=0
  • Upgrade golangci-lint action to v6
@github-actions github-actions bot added the protocol Protocol Team tickets label Jul 22, 2024
@boulder225
Copy link
Author

Hey team! Please add your planning poker estimate with Zenhub @gsora @KaloyanTanev @pinebit

@KaloyanTanev KaloyanTanev mentioned this issue Jul 22, 2024
Merged
obol-bulldozer bot pushed a commit that referenced this issue Jul 25, 2024
@KaloyanTanev
fixbuild: bump linter version (#3178)
54846a0 
golangci-lint was on older version that did not support go1.22 yet, which causes false alarms.

---

This update introduced a lot of new, deprecated and modified linters. There are a lot of changes in the files and I would advise for easier review to go over the commits, as I've tried to keep them concise, so there are only changes from one linter or if many, the changes are little. Most eye catching changes in the files are:

- `testifylint`: we had quite a lot of assertions inside of go routines, this is a bit of a red flag, because as observed in [this small example](https://go.dev/play/p/WoBGMiKQDEk), a failed assertion might or might not be caught
- The `golangci-lint` used in pre-commit was a default `golangci-lint` from [here](https://github.com/golangci/golangci-lint), using its default config, which made the linter pass locally all the time. This is something I've observed multiple times ([dating back to my very first PR](#2850 (comment)) :)). Now the pre-commit hook is running the locally installed `golangci-lint` tool and it checks if the version is the same as the one in the pipelines.
- Added `max-same-issues=0` and `max-issues-per-linter=0` so there is no limit on what we see in the output. Previously the default of 3 was used, so if there were hundreds of errors with one linter, it would have displayed only 3, which made it quite a hassle to fix.
- Bump the `golangci-lint` action's version to v6. It now points where exactly the issue is, which is great!

category: fixbuild
ticket: #3179
@KaloyanTanev KaloyanTanev self-assigned this Jul 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@KaloyanTanev KaloyanTanev

Labels
protocol Protocol Team tickets
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@KaloyanTanev @boulder225