add new domain names options #1106
Conversation
Introduce 2 new options: + `--allowed-domain-names` - remove given names from variable names' blacklist + `--forbidden-domain-names` - extends variable names' blacklist with given names The options listed above are used to create variable names' blacklist which is leveraged to detect `WrongVariableNameViolation` violations.
| @@ -109,6 +112,17 @@ def check_attribute_name(self, node: ast.ClassDef) -> None: | |||
| naming.UpperCaseAttributeViolation(target, text=target.id), | |||
| ) | |||
|
|
|||
| @property | |||
There was a problem hiding this comment.
It can be:
- Cached
- Moved to
logic/
There was a problem hiding this comment.
- Cached
Current implementation provide caching, it'a similar to @cached_property from django.
Do you want to introduce new dependency e.g. cached-property just for it?
- Moved to
logic/
I can move it there if you want, but could you tell me to which file or maybe I should create new one?
There was a problem hiding this comment.
No new dependecies are required, you can try to use lru_cache
There was a problem hiding this comment.
However when the code responsible for creating variable names' blacklist will be moved to logic/ then it should be enough to just assign the result in __init__:
def __init__(...):
...
self.variable_names_blacklist = logic.create_variable_names_blacklist()What do you think?
| *VARIABLE_NAMES_BLACKLIST, | ||
| *self._options.forbidden_domain_names, | ||
| } | ||
| for name in self._options.allowed_domain_names: |
There was a problem hiding this comment.
Can we use just set arithmetic here?
There was a problem hiding this comment.
Shouldn't be an error if allowed and forbidden names intersects?
There was a problem hiding this comment.
At least, if option variables intersect.
There was a problem hiding this comment.
Yes, I agree. But! Only during validation. Not in runtime
There was a problem hiding this comment.
is validate_options() proper place to add above mentioned validation logic?
Pull Request Test Coverage Report for Build 2496
💛 - Coveralls |
| @@ -45,7 +45,7 @@ | |||
| class _NameValidator(object): | |||
| """Utility class to separate logic from the naming visitor.""" | |||
|
|
|||
| _variable_names_blacklist: Optional[Set[str]] | |||
| variable_names_blacklist: Set[str] | |||
There was a problem hiding this comment.
To be honest I don't know :D I found it more proper/clear when I was writing this code. I will correct it in following commit
|
Merging into Thanks again! |
I have made things!
Checklist
CHANGELOG.mdRelated issue
Closes: #1103
Introduce 2 new options:
--allowed-domain-names- remove given names from variable names'blacklist
--forbidden-domain-names- extends variable names' blacklist withgiven names
The options listed above are used to create variable names' blacklist
which is leveraged to detect
WrongVariableNameViolationviolations.