-
Notifications
You must be signed in to change notification settings - Fork 87
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
Add variable names rule #283
Conversation
{ | ||
elvis_style, | ||
variable_naming_convention, | ||
#{regex => "^([A-Z][0-9a-zA-Z]*)*$", ignore => []} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you need the external star, i.e. you can get the same results with just [A-Z][0-9a-zA-Z]*
.
I'm also pretty sure your regex will render _ThisTotallyValidVariable
as invalid.
I think the proper default value should be along the lines of ^_?[A-Z][0-9a-zA-Z]*$
or maybe better we should ignore the leading _
when matching regexes in code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you're right, let me change it.
Don't forget to add the new rule to the wiki |
nomatch -> | ||
Msg = ?VARIABLE_NAMING_CONVENTION_MSG, | ||
Info = [VariableNameStr, Regex], | ||
Result = elvis_result:new(item, Msg, Info, 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please provide the location of the variable in the error message so that it is easier to find. You can check the following code here to know how to extract the location
information from a node.
Msg = ?VARIABLE_NAMING_CONVENTION_MSG, | ||
{Line, _} = ktn_code:attr(location, Variable), | ||
Info = [VariableNameStr, Line, Regex], | ||
Result = elvis_result:new(item, Msg, Info, 1), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should use Line
here, instead of 1
@harenson Since another PR was merged before this one, it is necessary to synchronize this branch with the |
@jfacorro Ok, I will take care of it in a while. |
@jfacorro @elbrujohalcon Thanks for your reviews 👍 |
@harenson Merged! 🎉 |
🎉 |
No description provided.