Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 11 commits into from
Jul 31, 2023

Conversation

comdiv
Copy link
Contributor

@comdiv comdiv commented Jul 17, 2023

it's improvement for #851 task
closes #851

}
}
}

func check(id *ast.Ident, thing string, w *lintNames) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It was too gready to "reserve" name check in package rule just for var-naming, moved lintNames as reviever to minimize exported named

@chavacava
Copy link
Collaborator

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
Copy link
Contributor Author

comdiv commented Jul 31, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

comdiv commented Jul 31, 2023

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
Copy link
Collaborator

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
Copy link
Contributor Author

comdiv commented Jul 31, 2023

Fix MD file too.

@chavacava chavacava merged commit 8941d19 into mgechev:master Jul 31, 2023
4 checks passed
@chavacava
Copy link
Collaborator

Thansk @comdiv !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add upperCaseConst to var-naming
2 participants