mgechev / revive

🔥 ~6x faster, stricter, configurable, extensible, and beautiful drop-in replacement for golint
https://revive.run
MIT License
4.74k stars 276 forks source link

imporve `var-naming` - add upperCaseConst option to allow UPPER_CASED constants #851 #852

Closed comdiv closed 1 year ago

comdiv commented 1 year ago

it's improvement for #851 task closes #851

chavacava commented 1 year ago

Hi @comdiv, thanks for the PR! (and sorry for the late response) I've made some little modifications to the code. Could you please provide a fix for the failing test case and update the description of the rule in RULES_DESCRIPTIONS.md ?

comdiv commented 1 year ago

Hi @comdiv, thanks for the PR! (and sorry for the late response) I've made some little modifications to the code. Could you please provide a fix for the failing test case and update the description of the rule in RULES_DESCRIPTIONS.md ?

Just not understand failing tests for now. Locally all work well.... Will try to fix

chavacava commented 1 year ago

Just not understand failing tests for now. Locally all work well.... Will try to fix

Test fails because I' ve added à test case for constant names without _ (e.g. VER, MAX, ...) You might need to pull locally the modifications I've made on your branch.

comdiv commented 1 year ago

Ok, pulled. But it's not after my changes, in legacy code we see:

if len(id.Name) >= 5 && allCapsRE.MatchString(id.Name) && strings.Contains(id.Name, "_") 

so your example of is valid alwaysVER neve cause fault, so i don't know what you are expected?

What is better:

  1. Change your VER example to VERSION one
  2. Remove len limit for uppercase totally
  3. Move len limit to parameter
  4. Make len 1 or 2?

Additionally this rule checks that name should contain undescores...

So for back compatibility and focus on task behind this MR, think that better is to fix test case?

chavacava commented 1 year ago

I see. The >= 5 condition comes from golint code base. Please remove the test case I' ve added and keep te condition as is. Thanks

comdiv commented 1 year ago

Fix MD file too.

chavacava commented 1 year ago

Thansk @comdiv !