var-naming: don't use leading k in Go names, vague and unclear failure description

[martinsirbe]

Seeing this linter failure message.

[2023-08-11T05:40:28.300Z] cmd/something/something.go:15:3: var-naming: don't use leading k in Go names; var kPath should be path (revive)

The issue with the failure message is that it effectively describes 'what' is wrong, but is vague on explaining 'why' it's wrong. Initially, after some research, I thought the problem might be related to the guideline in idiomatic Go which advises against Hungarian notation. However, upon closer inspection of the code, it's evident that the check is solely for variable names prefixed with a lowercase k.

revive/rule/var-naming.go

Lines 142 to 150 in b4fc3db

	if len(id.Name) > 2 && id.Name[0] == 'k' && id.Name[1] >= 'A' && id.Name[1] <= 'Z' {
	should := string(id.Name[1]+'a'-'A') + id.Name[2:]
	w.onFailure(lint.Failure{
	Failure: fmt.Sprintf("don't use leading k in Go names; %s %s should be %s", thing, id.Name, should),
	Confidence: 0.8,
	Node: id,
	Category: "naming",
	})
	}

It would be helpful to either annotate the code with comments or refine the failure message to clarify the 'why' behind this check within the var-naming rule.

As an alternative, perhaps the check should be designed to identify Hungarian notation. Or, maybe it's worth considering the removal of this check altogether?

[chavacava]

Hi @martinsirbe, thanks for filling the issue.
var-naming is a rule inherited from golint and its implementation is the (almost exact) copy of the original and because revive was conceived as a drop-in replacement of golint the failure message is identical to that generated by golint

[martinsirbe]

@chavacava thank you for providing that context. I understand the rationale behind retaining the original behaviour as revive is meant to be a drop-in replacement for golint. Based on the discussion in this GitHub issue it's been a source of confusion in the past. Even though the naming convention k[A-Z] might be rare, it could potentially lead to misunderstandings, as it doesn't explain 'why' the check is failing, nor it accurately implements Hungarian notation. Would it be possible to reconsider this specific rule or its messaging?

[chavacava]

Contributions to the README.md and or the rule's failure message are welcome.

[martinsirbe]

@chavacava first and foremost, I appreciate the openness to contributions on this matter. I concur that before diving into updating the README.md or the rule's failure message, it's essential to have a deeper understanding of the rationale behind this specific check. It's pivotal for clarity and to ensure that users comprehend the rule's intent.

While acknowledging that revive was designed as a drop-in replacement for golint, it might be beneficial to assess if staying true to golint's every detail, especially when it causes confusion, aligns with the broader goals of the project.

I believe there are two potential routes we can consider:

1. Refine the implementation to align more closely with the Hungarian notation, if that was the original intent.
2. Re-evaluate the rule's necessity and perhaps contemplate its removal or modification.

The emphasis here is on ensuring clarity and intuitiveness for users. I'm willing to contribute towards any decision made, whether it's to enhance the documentation or to make changes in the codebase.

[chavacava]

Being a drop-in replacement of golint means behave exactly as golint (for the good and the bad :) )
That said, now golint was deprecated long time ago and I think we can accept some differences with it (we already did it)
So, how to proceed?

1. Detecting Hungarian notation is not a matter of regexps (I think that's why golint never implemented such a rule) A quick scan of GO code bases shows a multitude of ids matching ^[a-z][A-Z][A-Za-z\d]*$ but not being Hungarian notation. I think trying to spot Hungarian notation will lead to more confusing failures (thus more issues in this repo :) )
2. Removing the detection of ids with the pattern ^k[A-Z][A-Za-z\d]*$ is something that can be justified: it tries to cope with just one particular case (k...) and it creates confusing false positives. I think we could remove the (sub)rule.