-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Add mixin-class-rgx
option
#5203
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
Changes from all commits
c984734
a3f7365
2b348f8
2fc500e
73521b3
04c06f7
3d415e1
649557d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -270,10 +270,13 @@ property-classes=abc.abstractproperty | |||||||
|
||||||||
[TYPECHECK] | ||||||||
|
||||||||
# Tells whether missing members accessed in mixin class should be ignored. A | ||||||||
# mixin class is detected if its name ends with "mixin" (case insensitive). | ||||||||
# Tells whether missing members accessed in mixin class should be ignored. | ||||||||
# A class is considered mixin if its name matches the mixin-class-rgx option. | ||||||||
ignore-mixin-members=yes | ||||||||
|
||||||||
# Regex pattern to define which classes are considered mixins. | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
@Pierre-Sassoulas Agree, but then let's add this. Perhaps we should also keep a list of exempted checks somewhere.. I don't know of a good place though. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe it should be a configurable option ? Also it seems we're skipping the check after doing all the calculations. We might want to skip the check as soon as we detect a mixin. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What about removing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Right, don't you prefer We already "deprecated" an option with blacklist/whitelist at some point #3961. I think the plan is to handle both until the end of time so users have no need to upgrade their configuration. There's no warning either for the same reason. Having to upgrade the configuration is bothersome with so many pylintrc in the wild already. I guess you had a different idea on how to do that ? I guess if the change isn't political it will attracts trolls less. I think that if we don't want to maintain all of the legacy options we might have to put a configuration upgrader into place (so the migration to the new configuration file is automated). It could work well with the need of using a single possible configuration file (instead of pylintrc, .pylintrc, setup.cfg, pyproject.toml, ~/.pylintrc) and it will probably be only There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
👍
I think we should only do a configuration upgrader after have moved to I intend to support both indeed, but with a deprecation warning when we notice There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sure, an upgrader wouldn't make sense as long as we still have optparse and aren't clear about what we want to deprecate. I'm not even sure if I want to have only pyporject.toml in 3.0. Just giving you the long term picture here :)
Sounds good. If the option is not being used by pylint but exists it's logical that we warn. Maybe we can suggest the option that replaced it in the warning so the user can copy paste the next conf ? It's not an upgrader, but it's close so we could create the base class for the upgrader and use it to get the recommendation ? :) Something like: class ConfigurationUpgrader:
def next_value(option_name:str, value: Any) -> [str, any]:
next_for_option_name = getattr(f"next_value_for_{option_name.replace('-', '_')}", None)
if next_for_option_name:
return next_for_option_name(value)
return option_name, value
def next_value_for_ignore_members(value: Any);
return 'mixin-class-rgx", [".*Mmixin"] There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I toyed around with this, but it turned out to be a little bit more difficult than I initially thought. They way we're loading options makes it really difficult to deprecate them and emit warnings for when users still use them. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for trying ! Do you think we should change the design of the options so that we're passing an object ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think ideally we wouldn't work with any I found that the I don't know what the initial objective of |
||||||||
mixin-class-rgx=.*MixIn | ||||||||
|
||||||||
# List of module names for which member attributes should not be checked | ||||||||
# (useful for modules/projects where namespaces are manipulated during runtime | ||||||||
# and thus existing member attributes cannot be deduced by static analysis) | ||||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,60 @@ | ||
"""Tests for the mixin-class-rgx option""" | ||
# pylint: disable=too-few-public-methods | ||
|
||
|
||
# Tests for not-async-context-manager | ||
|
||
|
||
class AsyncManagerMixedin: | ||
"""Class that does not match the option pattern""" | ||
|
||
def __aenter__(self): | ||
pass | ||
|
||
|
||
class AsyncManagerMixin: | ||
"""Class that does match the option pattern""" | ||
|
||
def __aenter__(self): | ||
pass | ||
|
||
|
||
async def check_not_async_context_manager(): | ||
"""Function calling the classes for not-async-context-manager""" | ||
async with AsyncManagerMixedin: # [not-async-context-manager] | ||
pass | ||
async with AsyncManagerMixin(): | ||
pass | ||
|
||
|
||
# Tests for attribute-defined-outside-init | ||
|
||
|
||
class OutsideInitMixedin: | ||
"""Class that does not match the option pattern""" | ||
|
||
def set_attribute(self): | ||
"""Set an attribute outside of __init__""" | ||
self.attr = 1 # [attribute-defined-outside-init] | ||
|
||
|
||
class OutsideInitMixin: | ||
"""Class that does match the option pattern""" | ||
|
||
def set_attribute(self): | ||
"""Set an attribute outside of __init__""" | ||
self.attr = 1 | ||
|
||
|
||
# Tests for no-member | ||
|
||
|
||
class NoMemberMixedin: | ||
"""Class that does not match the option pattern""" | ||
|
||
MY_CLASS = OutsideInitMixedin().method() # [no-member] | ||
|
||
class NoMemberMixin: | ||
"""Class that does match the option pattern""" | ||
|
||
MY_OTHER_CLASS = NoMemberMixin().method() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
[TYPECHECK] | ||
ignore-mixin-members=yes | ||
mixin-class-rgx=.*[Mm]ixin |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
not-async-context-manager:24:4:check_not_async_context_manager:Async context manager 'AsyncManagerMixedin' doesn't implement __aenter__ and __aexit__.:HIGH | ||
attribute-defined-outside-init:38:8:OutsideInitMixedin.set_attribute:Attribute 'attr' defined outside __init__:HIGH | ||
no-member:55:11::Instance of 'OutsideInitMixedin' has no 'method' member:INFERENCE |
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.
or
@Pierre-Sassoulas I meant either removing all checks for mixim's (so both
mixin_members
and theregex
) or only removing the members checkThere 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.
OK, I understand now. I'll try to see why it was added in the first place, there must be a reason.
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.
It's like that since the beginning of time and I don't see any explanation. 24e9d35#diff-76ab71e7eb3954c60b39e11f3dfe5cc45ee2b139c7738dfbca1566b8d50cf0b8R71
But now that I think of it, It might be to avoid false positive when the mixin is actually an async manager even if the current class is not. It's impossible to know that without analyzing the class that are supposed to be mixed. So in fact I think we should keep it. What do you think ?
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.
That is probably the reason why "they" did this initially. However, I think that might warrant a change of the
ignore_mixin_members
option name to something likeignore_mixin_class_checks
. That would be something for a different PR, but this name really does not fit what it is doing right now.Is there a "old-name" system for option names?
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 think we should use the new regex pattern only in order to check if a class is supposed to be a mixin and drop the other check.