Iterate over Keywords when using ClassDef.get_children

[cdce8p]

Description

This MR adds Keywords as a field to iterate over when getting the children of a ClassDef.

@hippo91 Would you mind taking a look at this? I'm a bit unsure how to best test this or if at all. In the end, it will be checked by pylint. The tests are passing with the current master.

Type of Changes

	Type
✓	🐛 Bug fix
✓	✨ New feature

Related Issue

Closes pylint-dev/pylint#3202

[Pierre-Sassoulas]

Nice fix, but could we had a little test case ? The example in pylint-dev/pylint#3202 seems on point.

[cdce8p]

Nice fix, but could we had a little test case ? The example in PyCQA/pylint#3202 seems on point.

@Pierre-Sassoulas I agree, but isn't that a pylint test? I plan to add that case to pylint anyway later, especially since I'll need to fix a pylint issue as a result of this MR.

from test_unused_import_const import DOMAIN

class Child:
    def __init_subclass__(cls, domain, **kwargs):
        pass

class Parent(Child, domain=DOMAIN):
    DOMAIN = DOMAIN  # add this line
    pass

After DOMAIN = DOMAIN is added the unused-import error reappears.

[Pierre-Sassoulas]

I think it's better if the faulty change in astroid are caught before they're merged in astroid's master. Of course the pylint test would catch it later but it's more complicated to fix, we'll have to create two issues and another MR in astroid instead of just fixing astroid's test in the base MR in the first place.

[cdce8p]

I'm not sure I fully understand what you meant. The MR itself is fine. Keywords should be iterated over when using get_children. The pylint issue I mentioned is just related to pylint. It was hidden previously since Keywords weren't checked.
The best I believe I can do here, is to test the Keywords are iterated over. The rest is up to pylint IMO.

Anyway, without a followup (the one I'm working on at the moment), we are trading a common false-positive against an uncommon one which can be fixed.

[Pierre-Sassoulas]

You're right we can't use the pylint example directly as a test. I think that a small test checking that we're getting the keyword when iterating on get_children is enough to prevent regression in astroid later on.

[cdce8p]

I think that a small test checking that we're getting the keyword when iterating on get_children is enough to prevent regression in astroid later on.

Added that one with the last commit: 973e22e

--

I have the fix for pylint finished already. Just need to do and write some more tests. Maybe hold off on merging until I open the pylint MR?

[hippo91]

@cdce8p i began to have a look at it today but won t have time to finish before next week end at best.
To be honest i don t know enough about this keyword member to have a clear idea.
Maybe you can give me hints about the way which leads you to this will to add keywords in the class children.
From what i have seen it is linked to metaclass but have you think about a way to handle __init_sublcass in a different manner?
I am sorry not to be able to be more precise. I need more time.

[cdce8p]

@hippo91 In class definitions it's possible to pass additional keyword arguments along that should be handled by the parent class with __init_subclass__. https://docs.python.org/3/reference/datamodel.html#object.__init_subclass__

The change does nothing more than iterating over all Keyword arguments which where previously just ignored.

[cdce8p]

@Pierre-Sassoulas I just opened pylint-dev/pylint#4207 to address the remaining pylint issue. The local tests pass. For CI you would need to merge this MR and release a new astroid version first. I would suggest 2.5.2?

[Pierre-Sassoulas]

I'll let @hippo91 review before merging. I don't see a problem with the changes but I'm not very relevant for astroid's MR. We can't release 2.5.2 yet anyway because of pylint-dev/pylint#4206 so we may as well wait for a proper review on this one.

[cdce8p]

I don't know if there is a solution for pylint-dev/pylint#4206 besides excluding the test. As far as I know this isn't a definite blocker, is it?

As for this MR, the fix is a definite improvement. I have been testing it with https://github.com/home-assistant/core which as a fairly large codebase and only noticed the error I fixed in pylint-dev/pylint#4207. It would remove around 90 pylint: disable comments. All in all, I would argue that it would be fine to merge it and do a fix later if necessary (which I don't believe will happen). It worked pretty well for #916 after all.

[cdce8p]

@Pierre-Sassoulas Would that work for you? I know a new release of pylint and astroid would be a huge help, especially because of the fixed old issues pylint-dev/pylint#3167 and now pylint-dev/pylint#3202.

[Pierre-Sassoulas]

Hmm, no I think it's a blocker, it means a crash is happening when we analyses typing.py from the standard lib.

[Pierre-Sassoulas]

I released 2.7.2 with this problem because I forgot to launch the acceptance test when releasing.

[hippo91]

@cdce8p thanks for this PR. I definitely think it is ok and an improvement!

[cdce8p]

@hippo91 Would you mind creating a new bugfix release of astroid, 2.5.3? Otherwise this fix can't be included in pylint.

[cdce8p]

I would assume that a fix for pylint-dev/pylint#4206 if there is any, would take at least a while longer.