Add markertypos linter for issue #23 #160
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: kannon92 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
cncf-cla: yes
kannon92
force-pushed
the
new-linter-markertypos
branch
3 times, most recently
from
18e64e9 to
9a5e699
Compare
|
|
||
| fieldMarkers := markersAccess.FieldMarkers(field) | ||
| for _, marker := range fieldMarkers.UnsortedList() { | ||
| checkMarkerSyntax(pass, marker) |
There was a problem hiding this comment.
If we are passing the actual Marker{} type from the markersAccess set here, I don't think that we need to validate the spacing issues.
It wouldn't have been successfully parsed if it didn't implement the correct marker semantics.
|
|
||
| typeMarkers := markersAccess.TypeMarkers(typeSpec) | ||
| for _, marker := range typeMarkers.UnsortedList() { | ||
| checkMarkerSyntax(pass, marker) |
There was a problem hiding this comment.
Ditto re: not needing to also validation spacing issues here.
| commonTypos := map[string]string{ | ||
| "kubebuidler": "kubebuilder", | ||
| "kubebuiler": "kubebuilder", | ||
| "kubebulider": "kubebuilder", | ||
| "kubbuilder": "kubebuilder", | ||
| "kubebulder": "kubebuilder", | ||
| "optinal": "optional", | ||
| "requied": "required", | ||
| "requird": "required", | ||
| "nullabel": "nullable", | ||
| "validaton": "validation", | ||
| "valdiation": "validation", | ||
| "defualt": "default", //nolint:misspell | ||
| "defult": "default", | ||
| "exampl": "example", | ||
| "examle": "example", | ||
| } |
There was a problem hiding this comment.
@JoelSpeed Is this what you had in mind for "common typos"? This seems like something that could get unruly fairly quickly and I'd advocate we avoid handling common misspellings of markers.
I wonder if something more along the lines of "is the parsed set of markers one that is a known marker?" would effectively achieve the same result while staying relatively maintainable?
There was a problem hiding this comment.
I wonder if something more along the lines of "is the parsed set of markers one that is a known marker?"
We can't claim to be complete in this regards though, so we do need to be able to handle unknown markers.
What is done here does align with the misspell marker, so I'm happy to keep this.
I was more thinking about the malformation stuff but this also seems valuable.
Additional things to think about
:=vs=, the former only being relevant tokubebuildermarkers- The value of the markers, e.g. is it a number, or string, should it have a value at all
- For markers that have required arguments (e.g. XValidation with
rule), are those arguments present - Case sensitivity
There was a problem hiding this comment.
Claude had some troubles with the first one so I reverted its attempt at fixing that one.
It could be worth iterating on these linters.
There was a problem hiding this comment.
As long as we capture from this conversation anything that doesn't merge in this PR and keep the issue open to contribute the rest later I think that's ok
Does mean that those enabling this will get a surprise when later new things pop up though
There was a problem hiding this comment.
Yea that is not a great experience.
One option could be including configurations on new changes. Essentially we could release new functionality into the linters via configurations so we could roll this out without breaking existing users.
I worry that this PR is already pretty large so not sure if we want to include all the items.
I don't really know how to solve all of these right now and I hope I can find time to get back to it.
But not sure the best path here.
| - **Space after '+' symbol**: Markers should not have space after the `+` symbol | ||
| - **Missing space after '//' prefix**: Markers should have a space after the `//` comment prefix |
There was a problem hiding this comment.
We need to make sure this doesn't interfere with other valid use cases. With that in mind, we need to be certain that the lines we are reporting on are intended to be one of the markers we are looking for. Starting my review on the docs so you may have handled this already
|
|
||
| ### Fixes | ||
|
|
||
| No automatic fixes are provided |
There was a problem hiding this comment.
Does this conflict with the Fixed versions line on 330? Looking at the implementation, it looks like we are suggesting fixes
There was a problem hiding this comment.
I thought I dropped the suggest fixes from this PR. I couldn't find what you mean though.
I had issues with suggesting fixes and I removed that code.
There was a problem hiding this comment.
The test suite you've added is running as if there are fixes applied 🤔
| // + required // want "marker should not have space after '\\+' symbol" | ||
| type SpacingIssueNonKubebuilder string | ||
|
|
||
| // +kubebuilder:validation:MaxLength=256 |
There was a problem hiding this comment.
Do we want to do anything about case sensitivity? I believe all of the markers in K8s are case-sensitive so perhaps folks are casing them wrong?
| // +kubebuilder:validation:MaxLength=256 | ||
| type KubebuilderWrongSyntax string | ||
|
|
||
| // +kubebuilder:validation:MinLength=1 |
There was a problem hiding this comment.
In the long term, do we want to make this marker understand the types that various markers should handle? E.g. if this isn't a number, return an error?
| // +default:value:="test" | ||
| type NonKubebuilderWrongSyntax string |
There was a problem hiding this comment.
Should this be an error?
| // +required:=true | ||
| // +optional:=false |
There was a problem hiding this comment.
Expecting errors here no?
There was a problem hiding this comment.
Would be good to explore some markers that aren't ones we care about, e.g. does this pick up markdown tables or golangci-lint nolint style markers
There was a problem hiding this comment.
Normally you'd see the want comments in the golden files too, I assume this won't pass tests with them missing
|
|
||
| // +kubebuilder:validation:Required | ||
| ValidField string `json:"validField"` | ||
| } No newline at end of file |
There was a problem hiding this comment.
Nit missing final new line char
| commonTypos := map[string]string{ | ||
| "kubebuidler": "kubebuilder", | ||
| "kubebuiler": "kubebuilder", | ||
| "kubebulider": "kubebuilder", | ||
| "kubbuilder": "kubebuilder", | ||
| "kubebulder": "kubebuilder", | ||
| "optinal": "optional", | ||
| "requied": "required", | ||
| "requird": "required", | ||
| "nullabel": "nullable", | ||
| "validaton": "validation", | ||
| "valdiation": "validation", | ||
| "defualt": "default", //nolint:misspell | ||
| "defult": "default", | ||
| "exampl": "example", | ||
| "examle": "example", | ||
| } |
There was a problem hiding this comment.
I wonder if something more along the lines of "is the parsed set of markers one that is a known marker?"
We can't claim to be complete in this regards though, so we do need to be able to handle unknown markers.
What is done here does align with the misspell marker, so I'm happy to keep this.
I was more thinking about the malformation stuff but this also seems valuable.
Additional things to think about
:=vs=, the former only being relevant tokubebuildermarkers- The value of the markers, e.g. is it a number, or string, should it have a value at all
- For markers that have required arguments (e.g. XValidation with
rule), are those arguments present - Case sensitivity
Implements a new linter that checks for common typos and syntax issues in marker comments, addressing the requirements from issue kubernetes-sigs#23: - Detects common typos in marker identifiers (kubebuidler → kubebuilder, optinal → optional, requied → required, etc.) - Validates spacing after '+' symbol (+kubebuilder not + kubebuilder) - Checks marker syntax conventions (kubebuilder markers use ':=', non-kubebuilder markers use '=') - Provides automatic fixes for all detected issues - Includes comprehensive test cases with valid and invalid marker examples The linter follows established patterns in the codebase and integrates with the existing marker analysis framework. Test coverage includes spacing issues, typo detection, syntax validation, and complex multi-issue scenarios. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
kannon92
force-pushed
the
new-linter-markertypos
branch
from
9a5e699 to
5ed3db3
Compare
4 participants
Fixes #23
Implements a new linter that checks for common typos and syntax issues in marker comments, addressing the requirements from issue #23:
The linter follows established patterns in the codebase and integrates with the existing marker analysis framework. Test coverage includes spacing issues, typo detection, syntax validation, and complex multi-issue scenarios.
🤖 Generated with Claude Code