Skip to content
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

Update regression tests for PyCQA/astroid#940 #4466

Merged

Conversation

nelfin
Copy link
Contributor

@nelfin nelfin commented May 11, 2021

Steps

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

Description

Changes to the inference of properties in a class context affected some existing tests

Type of Changes

Type
🐛 Bug fix

Related Issue

Ref pylint-dev/astroid#940
Depends on pylint-dev/astroid#941

Ref pylint-dev/astroid#940. These tests failed after fixing inference of
Enum.__members__ due to unexpected "not-iterable" warnings raised. These
warnings were not false-positives, as the test definition would have
raised TypeError at runtime: .keys and .values on dict/mappingproxy
are not properties, but methods.
Copy link
Contributor Author

@nelfin nelfin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following guard can actually be removed if Enum.__members__ is correctly inferred:

    if (
        owner.parent
        and isinstance(owner.parent, astroid.ClassDef)
        and owner.parent.name == "EnumMeta"
        and owner_name == "__members__"
        and node.attrname in ["items", "values", "keys"]
    ):
        # Avoid false positive on Enum.__members__.{items(), values, keys}
        # See https://github.com/PyCQA/pylint/issues/4123
        return False

Copy link
Member

@Pierre-Sassoulas Pierre-Sassoulas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you. Is there a reason why you did not remove the guard you detected ?

@coveralls
Copy link

Coverage Status

Coverage remained the same at 91.689% when pulling 60835c4 on nelfin:fix/astroid-940-regression-tests into 3fc4052 on PyCQA:master.

@nelfin
Copy link
Contributor Author

nelfin commented May 11, 2021

Is there a reason why you did not remove the guard you detected ?

To decouple this MR from the astroid changes. If I remove that guard, then this changeset should only be merged after pylint-dev/astroid#941, but if I leave it in then this can be merged before (since all the tests still pass) and that guard removed in a later change.

@Pierre-Sassoulas Pierre-Sassoulas merged commit 547ed55 into pylint-dev:master May 11, 2021
@Pierre-Sassoulas
Copy link
Member

Thank you for the explanation :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants