linter: add `namingconventions` linter #106

everettraven · 2025-06-25T19:55:08Z

Adds a new linter, namingconventions, that allows for configuration of custom naming conventions to be enforced on both Go type and serialized field names.

Additionally, this PR updates the nophase and notimestamp linters to wrap the namingconventions linter similarly to how the nonullable linter wraps the forbiddenmarkers linter.

This should make future default field naming convention linters much easier to write, while giving users flexibility to add their own custom naming conventions.

everettraven · 2025-06-25T19:56:27Z

Just a PoC, not mergeable

/hold

everettraven · 2025-06-25T19:58:38Z

pkg/analysis/fieldnaming/analyzer.go

+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
+}


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.

everettraven · 2025-06-25T20:02:48Z

/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.

Karthik-K-N · 2025-06-26T02:53:04Z

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.

JoelSpeed · 2025-06-27T10:17:10Z

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

everettraven · 2025-09-18T17:52:17Z

pkg/analysis/forbiddenmarkers/analyzer.go

+
+

TODO: revert this

that allows for defining custom namingconventions to be enforced on Go and serialized field names Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

everettraven · 2025-09-19T20:47:31Z

@JoelSpeed I've spent some time updating this and think this is ready for a review.

everettraven · 2025-09-22T13:05:24Z

/hold cancel

docs/linters.md

JoelSpeed · 2025-09-22T16:18:15Z

docs/linters.md

+linterConfig:
+  namingconventions:
+    conventions: 
+      - name: englishcolour 


Nit, probably BritishEnglishColour? Is casing meaningful here?

I can change it.

Casing will matter here - I'll update the documentation on the field and docs.

JoelSpeed · 2025-09-22T16:22:04Z

pkg/analysis/namingconventions/analyzer.go

+	suggestedFixes := []analysis.SuggestedFix{}
+	suggestedFixes = append(suggestedFixes, analysis.SuggestedFix{
+		Message: "remove the field",
+		TextEdits: []analysis.TextEdit{
+			{
+				Pos:     field.Pos(),
+				NewText: []byte(""),
+				End:     field.End(),
+			},
+		},
+	})


Wondering if we need to declare an empty list first?

Suggested change

suggestedFixes := []analysis.SuggestedFix{}

suggestedFixes = append(suggestedFixes, analysis.SuggestedFix{

Message: "remove the field",

TextEdits: []analysis.TextEdit{

{

Pos: field.Pos(),

NewText: []byte(""),

End: field.End(),

},

},

})

suggestedFixes := append([]analysis.SuggestedFix{}, analysis.SuggestedFix{

Message: "remove the field",

TextEdits: []analysis.TextEdit{

{

Pos: field.Pos(),

NewText: []byte(""),

End: field.End(),

},

},

})

Or

Suggested change

suggestedFixes := []analysis.SuggestedFix{}

suggestedFixes = append(suggestedFixes, analysis.SuggestedFix{

Message: "remove the field",

TextEdits: []analysis.TextEdit{

{

Pos: field.Pos(),

NewText: []byte(""),

End: field.End(),

},

},

})

suggestedFixes := []analysis.SuggestedFix{

{

Message: "remove the field",

TextEdits: []analysis.TextEdit{

{

Pos: field.Pos(),

NewText: []byte(""),

End: field.End(),

},

},

},

}

JoelSpeed · 2025-09-22T16:29:17Z

pkg/analysis/namingconventions/config.go

+	// violationMatcher is a required regular expression
+	// used to identify violating portions of a field name.


Is there a particular regex library we are using? Should we document which regex version we are compatible with? Go's standard library regix is RE2 compatible IIRC

We are using Go's standard library - I'll update this documentation

JoelSpeed · 2025-09-22T16:37:42Z

pkg/analysis/notimestamp/testdata/src/a/a.go


 type NoTimeStampTestStruct struct {
 	// +optional
-	TimeStamp *time.Time `json:"timeStamp,omitempty"` // want "field TimeStamp: prefer use of the term time over timestamp"


When we don't have field <> in the name, it makes exceptions hard to craft in the golangci-lint file, I think ideally we would restore this part

JoelSpeed · 2025-09-22T16:38:42Z

pkg/analysis/notimestamp/analyzer_test.go

 */

-package notimestamp_test
+package notimestamp


We don't normally need to do this, we use the public Initializer() for the test normally

JoelSpeed · 2025-09-22T16:41:50Z

pkg/analysis/notimestamp/analyzer.go

+				ViolationMatcher: "(?i)timestamp",
+				Operation:        namingconventions.OperationReplace,
+				Message:          "prefer use of the term 'time' over 'timestamp'",
+				Replace:          "Time",


Replacement?

It has only just clicked to me that this is the new string, I thought we were replacing this initially.

IIUC, the section that matches the ViolationMatcher gets replaced with this string right?

Replacement?

It has only just clicked to me that this is the new string, I thought we were replacing this initially.

Fair enough. I used replace because the operation is Replace, but I can change both to "replacement" if that would be more clear.

IIUC, the section that matches the ViolationMatcher gets replaced with this string right?

Correct

JoelSpeed · 2025-09-22T16:43:34Z

pkg/analysis/nophase/analyzer_test.go

 limitations under the License.
 */
-package nophase_test
+package nophase


Should be able to use the public Initializer() function rather than changing this

JoelSpeed · 2025-09-22T16:46:08Z

pkg/analysis/namingconventions/namingconventions_suite_test.go

+	. "github.com/onsi/gomega"
+)
+
+func TestConditions(t *testing.T) {


Suggested change

func TestConditions(t *testing.T) {

func TestNamingConventions(t *testing.T) {

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

JoelSpeed · 2025-09-24T09:43:33Z

/approve
/lgtm

k8s-ci-robot · 2025-09-24T09:43:42Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: everettraven, JoelSpeed

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [JoelSpeed]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-ci-robot requested review from jpbetz and sivchari June 25, 2025 19:55

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 25, 2025

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 25, 2025

everettraven commented Jun 25, 2025

View reviewed changes

everettraven force-pushed the linter/fieldnaming branch from a05f7c4 to 67aa900 Compare September 17, 2025 19:43

k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Sep 18, 2025

everettraven commented Sep 18, 2025

View reviewed changes

pkg/analysis/forbiddenmarkers/analyzer.go Outdated

Comment on lines 58 to 59

Copy link

Contributor Author

everettraven Sep 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TODO: revert this

everettraven force-pushed the linter/fieldnaming branch from d386eb7 to 1ad36f3 Compare September 19, 2025 20:42

k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Sep 19, 2025

everettraven added 3 commits September 19, 2025 16:44

linters: add namingconventions linter

fcbf6de

that allows for defining custom namingconventions to be enforced on Go and serialized field names Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

nophase: wrap namingconventions linter

5180baf

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

notimestamp: wrap namingconventions linter

013a77c

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

everettraven force-pushed the linter/fieldnaming branch from 1ad36f3 to 013a77c Compare September 19, 2025 20:44

everettraven changed the title ~~poc: generic naming convention linter~~ linter: add namingconventions linter Sep 19, 2025

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 22, 2025

JoelSpeed reviewed Sep 22, 2025

View reviewed changes

review: updates based on review from Joel

8d19d9e

Signed-off-by: Bryce Palmer <bpalmer@redhat.com>

everettraven force-pushed the linter/fieldnaming branch from 2b67e7b to 8d19d9e Compare September 23, 2025 11:34

k8s-ci-robot assigned JoelSpeed Sep 24, 2025

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 24, 2025

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2025

k8s-ci-robot merged commit 502783c into kubernetes-sigs:main Sep 24, 2025
4 checks passed

		// violationMatcher is a required regular expression
		// used to identify violating portions of a field name.

	func TestConditions(t *testing.T) {
	func TestNamingConventions(t *testing.T) {

linter: add namingconventions linter #106

linter: add namingconventions linter #106

Conversation

everettraven commented Jun 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

everettraven commented Jun 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

everettraven commented Jun 25, 2025

Uh oh!

Karthik-K-N commented Jun 26, 2025

Uh oh!

JoelSpeed commented Jun 27, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

everettraven commented Sep 19, 2025

Uh oh!

everettraven commented Sep 22, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

JoelSpeed commented Sep 24, 2025

Uh oh!

k8s-ci-robot commented Sep 24, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

linter: add `namingconventions` linter #106

linter: add `namingconventions` linter #106

everettraven commented Jun 25, 2025 •

edited

Loading