Add mixin-class-rgx option

[DanielNoord]

• Add yourself to CONTRIBUTORS if you are a new contributor.
• Add a ChangeLog entry describing what your PR does.
• If it's a new feature, or an important bug fix, add a What's New entry in
  doc/whatsnew/<current release.rst>.
• Write a good description on what the PR does.

Type of Changes

	Type
✓	✨ New feature
✓	📜 Docs

Description

Adds a new option mixin-class-rgx which allows users to set a pattern that their mixin class names should adhere to.

I think we might need to reconsider whether the ignore-mixin-members option should be checked for the not-async-context-manager and attribute-defined-outside-init messages. I've added tests for them because it currently is, but the name of that option and the description of it doesn't really imply it should.

Supersedes #3719

[coveralls]

Pull Request Test Coverage Report for Build 1377657716

• 9 of 9 (100.0%) changed or added relevant lines in 3 files are covered.
• No unchanged relevant lines lost coverage.
• Overall coverage increased (+0.001%) to 93.297%

---

Totals
Change from base Build 1377628875:	0.001%
Covered Lines:	13681
Relevant Lines:	14664

---

💛 - Coveralls

[Pierre-Sassoulas]

👍 Nice option !

[DanielNoord]

@Pierre-Sassoulas What do you think about the question posed in the PR description?

[Pierre-Sassoulas]

I think we might need to reconsider whether the ignore-mixin-members option should be checked for the not-async-context-manager and attribute-defined-outside-init messages. I've added tests for them because it currently is, but the name of that option and the description of it doesn't really imply it should.

I think we're doing that because a Mixin need to have attribute defined outside init so it would be a false positive if we raise a warning for that in a mixin ? For not-async-context-managerI don't know why it has something to do with mixin. Is there another configuration we could use for mixin ? Or do you want an option more explicit than ignore-mixin- members ? Or I misunderstood what you meant ?

[DanielNoord]

Ah yeah, I forgot about the outside of __int__ case.
However, for async I don't know why it depends on ignore-members. Can we remove it?

[Pierre-Sassoulas]

Can we remove it?

After reading the code I think someone thought ignore mixin was an option to ignore everything for mixin ? Must have had a very troublesome mixin class 😄 I think we can remove, thank you for catching that problem.

[DanielNoord]

I will submit a different PR to remove it and then remove the tests here.

Edit: @Pierre-Sassoulas do you want me to remove the check for Mixin from not-async-context-manager altogether? Or only the check for _ignore_mixin_members?

[Pierre-Sassoulas]

do you want me to remove the check for Mixin from not-async-context-manager altogether? Or only the check for _ignore_mixin_members?

I did not see the check for Mixin from not-async-context-manager or I don't understand exactly what you meant. I think we can remove everything as I don't see how Mixin are linked to async manager (?) But I don't know exactly what you're suggesting to delete.

[DanielNoord]

Suggested change

	# Ignore mixin classes if they match the rgx option.
	if self._ignore_mixin_members and self._mixin_class_rgx.match(
	inferred.name
	):
	continue
	# Ignore mixin classes if they match the rgx option.
	if self._mixin_class_rgx.match(
	inferred.name
	):
	continue

or

Suggested change

	# Ignore mixin classes if they match the rgx option.
	if self._ignore_mixin_members and self._mixin_class_rgx.match(
	inferred.name
	):
	continue

@Pierre-Sassoulas I meant either removing all checks for mixim's (so both mixin_members and the regex) or only removing the members check

[Pierre-Sassoulas]

OK, I understand now. I'll try to see why it was added in the first place, there must be a reason.

[Pierre-Sassoulas]

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 ?

[DanielNoord]

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 like ignore_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?

[Pierre-Sassoulas]

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.

[DanielNoord]

Suggested change

	# Regex pattern to define which classes are considered mixins.
	# Regex pattern to define which classes are considered mixins and should be
	exempted from certain checks.

@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.

[Pierre-Sassoulas]

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.

[DanielNoord]

What about removing ignore-members and adding a new option ignore-checks-for-mixin which would be a comma separated list of checks that exclude Mixin? We could add the supported checks to the description of that option.
I think I have already thought of good way to deprecate an option.

[Pierre-Sassoulas]

Right, don't you prefer ignored-checks-for-mixin though ?

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 pyproject.toml in the long term.

[DanielNoord]

Right, don't you prefer ignored-checks-for-mixin though ?

👍

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 pyproject.toml in the long term.

I think we should only do a configuration upgrader after have moved to argparse.

I intend to support both indeed, but with a deprecation warning when we notice ignore-members is being used.

[Pierre-Sassoulas]

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 :)

I intend to support both indeed, but with a deprecation warning when we notice ignore-members is being used

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"]

[DanielNoord]

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.
They need to have an optdict in one of the checkers, but then we also load the option every time ourselves. Catching the option and changing it directly to the new name is also difficult as we would need to create combinations of every validator type with every other validator type (ie., to be able to translate a "yes-no" option to a "list of strings" option.
I think we should leave this as is, open an issue to track this and wait for the move to argparse. I hope that module provides better ways to do this.

[Pierre-Sassoulas]

Thank you for trying ! Do you think we should change the design of the options so that we're passing an object (argparse.Namespace ?) to the checker at initialization ?

[DanielNoord]

I think ideally we wouldn't work with any OptionsProviders at all and just load all possible options from a large dict of optdicts (or Arguments as suggested in #5152) at start up once and subsequently load the user defined options/overwrites.

I found that the load_defaults function was called 2/3 times for one single test and the whole concept of OptionsProviders creates numerous problems when different checker classes need to access the same option. (See #5195).
As indicated in #4814 option processing take a considerable time for our start-up, which I can imagine is because of stuff like calling load_defaults twice.
The get_global_option function is basically trying to circumvent this design of OptionProviders, as it turns out it is often much more logical to make options "global" for all checkers.

I don't know what the initial objective of OptionProviders was and I think we will likely run into unexpected problems when we would move to such a design, but from my experience with our config module this might in the end be much more maintainable.