Custom ci lints#4841
Conversation
|
Adding a note here to cross-reference this PR with two other open PRs which introduce changes to golangci-lint this PR will likely conflict with. |
|
Dependency limit exceeded — report not shown. This pull request scan exceeded the 10,000-dependency limit applied to this scan, so the results are incomplete and may be inaccurate. To avoid reporting false positives, Socket has not posted a report. Upgrade your plan to raise the dependency limit and get complete reports, or view the partial scan in the dashboard. Socket is always free for open source. If this is a non-commercial open source project, contact us to request a free Team account. |
johnsaigle
left a comment
There was a problem hiding this comment.
The linter works and I tested the new lint make targets from each of the subdirectories and via the Docker method.
I left some comments mostly around deleting a custom caching system which I don't believe is necessary given Go's built-in caching mechanism.
I also left several notes marked "suggestion" that can be ignored/resolved without comment. 👍🏻
89a6c2b to
70b261b
Compare
johnsaigle
left a comment
There was a problem hiding this comment.
Ran the tests on the latest commit with and without channelcheck enabled.
djb15
left a comment
There was a problem hiding this comment.
I didn't review the linter itself in a tonne of detail but I think the changes to the existing code look good, seems to be pretty unobtrusive and it also doesn't seem to change the developer workflow at all which is great.
The linter itself doesn't appear to do anything dangerous and the test cases cover the channel use cases we have in the monorepo so far.
|
I messed up the git history' commits were duplicated for some reason. Changes are much cleaner now. |
99393cc to
de9e4ab
Compare
|
@mdulin2 looks like you need to rebase after your EVM watcher PR got merged |
27d42f1 to
8cbdb3c
Compare
| standalone analyzer flag (when running the `wormhole-lint` binary, using the | ||
| **Flag** name). | ||
|
|
||
| | Setting | Flag | Type | Default | Description | |
There was a problem hiding this comment.
Should we include CheckEmptyDefault in the table?
There was a problem hiding this comment.
@mdulin2 feel free to mark my 2 comments as resolved and ignore, they are non blockers.
|
|
||
| `channelcheck` settings: | ||
|
|
||
| | Setting | Type | Default | Description | |
There was a problem hiding this comment.
Might be better to use actual setting names (CheckBlockingSends) instead of flags (blocking)?
SEJeff
left a comment
There was a problem hiding this comment.
Why not use golangci-lint? I am unclear on why we want this vs using the industry standard linting tool and plugging any custom linters we want into it.
|
@SEJeff This is how you do that. https://golangci-lint.run/docs/plugins/module-plugins/ I agree it's a weird system that they make you recompile a custom version of golangci-lint but that appears to be the way this gets done. We also took a look at using revive instead but it has basically the same set-up, so we went this way. |
|
Any thoughts from folks on this? I'd love to get this merged so that we can turn on the channel linter in the repo, as well as add other custom linters. |
|
@mdulin2 I think we're good on our side, you just need codeowner review now |
The PR is introducing the capability for custom linters in Wormhole:
linterschannelchecklinter but doesn't enable it.channelcheckis NOT enabled in this PR to limit the number of files being touched. This will be enabled in a separate follow-up PR. There are comments labeledNote on channel capacitythat will be converted either intonolintcomments or into code that satisfies the linter.