-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
golangci: fix invalid linter-settings
configuration name
#7244
Conversation
✅ Deploy Preview for openpolicyagent ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See the misspell
issues here: https://github.com/open-policy-agent/opa/actions/runs/12612660972/job/35149642609?pr=7244#step:3:56
The issues in the log are not exhaustive. Note the message in the CI log: level=info msg="[runner/max_from_linter] 89/139 issues from linter misspell were hidden, use --max-issues-per-linter"
That's a good catch! Thank you for that 👍 Just quickly skimming the issues reported and many/most seem to be around just differences in US English vs British English. While I think we've said the docs should stick to US English, I don't think we want to enforce that in code comments. If we enable this linter properly, I think we'll want to have it accept both if possible. |
@anderseknert Hi, the There doesn't seem to be a way to tell |
I didn't mean to say it should ignore comments, but that both US and UK locales should be considered OK outside of the docs.
From those docs though, it sounds like we can just remove the |
I have already made this change in the second commit of this PR 8f329cb 😄 |
Awesome! Why is the build green now though? Call me cynical, but I can't imagine we don't have a single typo or misspelling in any of the comments in the OPA codebase 😄 |
😁 We really didn’t have any typos apart from the US misspellings. I added the Click to show long output
To make sure that Line 72 in 3568eab
And |
Not saying it's on you to fix it, and we should probably merge this anyway — fixing an obvious error in the config is a good thing no matter how you look at it. But just to provide the first example I could find:
From here. This should be "interpreted" with a single t in both locales, shouldn't it? There must be hundreds if not thousands of issues with spelling in our comments 😅 And I don't particularly like the idea of a typo i a comment failing a build... but that leaves the question about what this linter actually does, if it actually does anything. But oh well, it's an approve from me, and perhaps others have more knowledge to share here. |
🥹 Yep, I jumped to a conclusion too fast. I ran a full typo check on the codebase using GoLand. The inspection was much more thorough than |
`linters-settings` is the correct configuration name [1]. [1]: https://golangci-lint.run/usage/configuration/#linters-settings-configuration Signed-off-by: Eng Zer Jun <[email protected]>
Signed-off-by: Eng Zer Jun <[email protected]>
Good to learn for us all! And certainly not something that should block an obvious fix like this. So let's get this in, and we can consider what a dictionary like that should look like, if we even want one. I remember @msorens having made contributions to automate spellchecking in the past, so tagging him for advice 🙂 Merging this as soon as the build passes though. |
Why the changes in this PR are needed?
We started using
golangci-lint
in PR #3465. However, the configuration namelinter-settings
is incorrect, which caused the configuration for thelll
andmisspell
linters to not be applied.opa/.golangci.yaml
Lines 140 to 145 in 3568eab
The correct configuration name is
linters-settings
, as noted in https://golangci-lint.run/usage/configuration/#linters-settings-configurationDue to this configuration error, we have accumulated many US spelling errors in the codebase. Some of these would cause breaking changes for existing users because they require renaming functions and error strings.
Tip
Hyrum's law
https://github.com/golang/go/blob/go1.23.4/src/net/http/request.go#L1208
What are the changes in this PR?
linter-settings
tolinters-settings
in.golangci.yaml
misspell
linter. This is because renaming functions and error strings are considered breaking changesNotes to assist PR review:
Further comments: