poc: generic naming convention linter #106
Conversation
Signed-off-by: Bryce Palmer <bpalmer@redhat.com>
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: everettraven 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
|
Just a PoC, not mergeable /hold |
k8s-ci-robot
added
the
do-not-merge/hold
| var defaults = []NamingConvention{ | ||
| { | ||
| Matcher: *regexp.MustCompile("(?i)phase"), | ||
| Operation: "Drop", | ||
| Message: "phase fields are deprecated and discouraged. conditions should be used instead.", | ||
| }, | ||
| { | ||
| Matcher: *regexp.MustCompile("(?i)timestamp"), | ||
| Operation: "Replace", | ||
| Message: "prefer use of the term 'time' over 'timestamp'", | ||
| // replacement of `Time` follows CamelCase principles for field names and JSON tags | ||
| // TODO: Handle case where it is _not_ the second word in CamelCase for json tag | ||
| Replacement: "Time", | ||
| }, | ||
| { | ||
| Matcher: *regexp.MustCompile("(?i)reference"), | ||
| Operation: "Replace", | ||
| Message: "prefer use of the term 'ref' over 'reference'", | ||
| // replacement of `Ref` follows CamelCase principles for field names and JSON tags | ||
| // TODO: Handle case where it is _not_ the second word in CamelCase for json tag | ||
| Replacement: "Ref", | ||
| }, | ||
| } | ||
|
|
||
| type NamingConvention struct { | ||
| // Matcher is a regular expression | ||
| // used to identify field names | ||
| // where this convention applies | ||
| Matcher regexp.Regexp | ||
|
|
||
| // Replacement is an optional | ||
| // string value used to replace the matched content | ||
| // in a suggested fix. | ||
| // Only used when Operation is Replace. | ||
| Replacement string | ||
|
|
||
| // Operation is the type of operation that should take place for this | ||
| // naming convention. | ||
| // One of Drop, Replace. | ||
| Operation string | ||
|
|
||
| // Message is the message that should be included in the | ||
| // linter report when this naming convention is applied. | ||
| Message string | ||
| } |
There was a problem hiding this comment.
If we were to continue with this approach, the thinking is that we would implement a configuration layer for this linter that exposes this NamingConvention type to allow the development of custom naming conventions.
We could also add a configuration option to include/exclude the defaults we have put in place based on the Kubernetes API conventions doc.
|
/cc @JoelSpeed @sivchari - I'm curious about your thoughts here. @Karthik-K-N - just a heads up since you have #105 open and I took some liberties to include that convention in this example as well. I don't think this should block your PR from going in (or supersede it). Also happy to hear your thoughts if you have any here. |
|
Even I thought a bit about covering multiple linters under a cover, We have few linters doing similar things like nophase, notimestamp, noduration. Only thought came to my mind at that time was, what if user wants to relax one among many linterns. But with your approach of having a configuration file would be handy at that time. |
|
I'm a fan of the approach being taken for the NoNullable linter, where we still have a top level linter that contains the descriptions and reasoning of why we do/don't do certain things, but under the hood is just configuring another linter. Perhaps we can do similar here |
just a proof-of-concept to show an idea of what a generic field naming convention linter could look like