Add an ignore list to MixedNames #311
Conversation
There was a problem hiding this comment.
Hi @EduardoVaz06, thanks for making this contribution! I agree with your reasoning and think it's definitely appropriate to include in the core rule behaviour.
Implementation looks great, no problems I can see. I have a couple of suggestions for some extra tests.
| .appendImpl("begin") | ||
| .appendImpl(" b_IgnoreBool := True;") | ||
| .appendImpl("end;")) | ||
| .verifyNoIssues(); |
There was a problem hiding this comment.
Could you add a test that asserts that the exclusion check is not case-sensitive? e.g. having the excluded name be MyIdentifierName, but the declaration be MYIDENTIFIERNAME and the usage be myidentifiername.
|
|
||
| class MixedNamesCheckTest { | ||
| private static DelphiCheck createCheck() { | ||
| MixedNamesCheck check = new MixedNamesCheck(); | ||
| check.excludedNames = "b_IgnoreBool"; |
There was a problem hiding this comment.
I'm not sure what this b_ prefix is here - is it a Hungarian notation for boolean variables?
Since identifiers with this prefix is unconventional and would be caught by ClassName, VariableName, etc. it might make people reading this code wonder if there's a specific reason we're testing with this prefix. Could we change this variable name to something else?
| @RuleProperty( | ||
| key = "excludedNames", | ||
| description = "List of names to ignore, separated by a comma.") | ||
| public String excludedNames = ""; |
There was a problem hiding this comment.
Could you add a test asserting that you can provide multiple comma-separated names?
This PR adds a setting on MixedNames rule to ignore certain names.
This PR implements a suggestion I have for MixedNames rule. In my company, we have certain patterns in names, so in some cases it's suggested by the rule to comply with the definition, and ends up not being compliant with our pattern. I think it would be good to have a list for names to be ignored.
For example, sometimes the rules suggest to be WParam, and sometimes WPARAM, due to the definition. If we have a pattern to always be WParam, this addition could help to ignore WParam name, and don't raise any issues.
It's up to you to know if this is a desired feature or not.