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

Add documentation examples for overridden-final-method checker #6080

Conversation

mbyrnepr2
Copy link
Member

Type of Changes

Type
🐛 Bug fix
✨ New feature
🔨 Refactoring
📜 Docs

Description

Partially closes issue #5953

@mbyrnepr2 mbyrnepr2 marked this pull request as draft March 31, 2022 18:45
@mbyrnepr2 mbyrnepr2 force-pushed the overridden-final-method-documentation branch from 0f8a264 to 09aa0aa Compare March 31, 2022 18:52
@mbyrnepr2
Copy link
Member Author

Thanks for the reviews @DanielNoord @Pierre-Sassoulas. I need to revisit since the CI has found an issue.

@DanielNoord
Copy link
Collaborator

Could be a check-messages issue again..

@mbyrnepr2
Copy link
Member Author

Could be a check-messages issue again..

Thanks. I had the same thought since reading the issue created by Andreas.

@Pierre-Sassoulas
Copy link
Member

I did not expect #5956 to catch so many hard to find bug, but this PR has already been well worth the hassle.

@DanielNoord
Copy link
Collaborator

This is interesting... Are we getting a crash because we previously never entered that function?

@mbyrnepr2
Copy link
Member Author

The visit function? I imagine that has been working ok since reproducing the emitted warning is as expected in the functional tests. Or is this something else? I haven't dug into what the current failing test in the CI is checking, does it expect every possible message to be present in the decorator?

@DanielNoord
Copy link
Collaborator

The visit function? I imagine that has been working ok since reproducing the emitted warning is as expected in the functional tests. Or is this something else? I haven't dug into what the current failing test in the CI is checking, does it expect every possible message to be present in the decorator?

Yes, if there is a check_messages decorator then all messages should be in it. The check goes something like if not decorated or (decorated and msg in decorated.names): do check if you understand what I mean.

@mbyrnepr2 mbyrnepr2 force-pushed the overridden-final-method-documentation branch from 103cd53 to d11b0c8 Compare April 1, 2022 17:20
@mbyrnepr2
Copy link
Member Author

@DanielNoord I don't see anything obviously incorrect with respect to the setup of the various check_messages decorators in the code.

I've noticed that locally on my Windows machine the test which is failing in the CI passes. Instead I see this one:

    def _runTest(self) -> None:
        """Run the test and assert message differences."""
        self._linter.check([str(self._test_file[1])])
        expected_messages = self._get_expected()
        actual_messages = self._get_actual()
>       assert expected_messages == actual_messages
E       AssertionError: assert Counter({(1, ...builtin'): 1}) == Counter()
E         Left contains 1 more item:
E         {(1, 'bad-builtin'): 1}
E         Use -v to get more diff

pylint\doc\test_messages_documentation.py:111: AssertionError
=========================== short test summary info ===========================
FAILED pylint\doc\test_messages_documentation.py::test_code_examples[bad-builtin-bad]
======================== 1 failed, 97 passed in 20.53s ========================

Failing CI test:

def _runTest(self) -> None:
        """Run the test and assert message differences."""
        self._linter.check([str(self._test_file[1])])
        expected_messages = self._get_expected()
        actual_messages = self._get_actual()
>       assert expected_messages == actual_messages
E       AssertionError: assert Counter({(11,...-method'): 1}) == Counter()
E         Left contains 1 more item:
E         {(11, 'overridden-final-method'): 1}
E         Full diff:
E         - Counter()
E         + Counter({(11, 'overridden-final-method'): 1})

doc/test_messages_documentation.py:111: AssertionError
=========================== short test summary info ============================
FAILED doc/test_messages_documentation.py::test_code_examples[overridden-final-method-bad]
======================== 1 failed, 97 passed in 10.[44](https://github.com/PyCQA/pylint/runs/5799254136?check_suite_focus=true#step:6:44)s =========================

@DanielNoord
Copy link
Collaborator

@mbyrnepr2
Could this be the issue:

pylint /Users/daniel/DocumentenLaptop/Programming/Github/pylint/doc/data/messages/o/overridden-final-method/bad.py
************* Module bad
doc/data/messages/o/overridden-final-method/bad.py:5:5: W2602: typing.final is not supported by all versions included in the py-version setting (using-final-decorator-in-unsupported-version)

------------------------------------------------------------------
Your code has been rated at 8.57/10 (previous run: 0.00/10, +8.57)

1 similar comment
@DanielNoord
Copy link
Collaborator

@mbyrnepr2
Could this be the issue:

pylint /Users/daniel/DocumentenLaptop/Programming/Github/pylint/doc/data/messages/o/overridden-final-method/bad.py
************* Module bad
doc/data/messages/o/overridden-final-method/bad.py:5:5: W2602: typing.final is not supported by all versions included in the py-version setting (using-final-decorator-in-unsupported-version)

------------------------------------------------------------------
Your code has been rated at 8.57/10 (previous run: 0.00/10, +8.57)

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.

Animal example. It was either that or a fruits example 😄

Comment on lines 4 to 12
class Base:
@final
def my_method(self):
pass


class Subclass(Base):
def my_method(self): # [overridden-final-method]
pass
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class Base:
@final
def my_method(self):
pass
class Subclass(Base):
def my_method(self): # [overridden-final-method]
pass
class Animal:
@final
def breath(self):
return True
class Cat(Animal):
def breath(self): # [overridden-final-method]
pass

Copy link
Member Author

Choose a reason for hiding this comment

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

Quite an intuitive example @Pierre-Sassoulas.
I've changed the naming slightly.

Comment on lines 4 to 12
class Base:
@final
def my_method(self):
pass


class Subclass(Base):
def my_other_method(self):
pass
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
class Base:
@final
def my_method(self):
pass
class Subclass(Base):
def my_other_method(self):
pass
class Animal:
@final
def breath(self):
return True
class Cat(Animal):
def purr(self):
return True

@mbyrnepr2 mbyrnepr2 force-pushed the overridden-final-method-documentation branch from 5ee5eb7 to cc4e26a Compare April 5, 2022 10:23
@mbyrnepr2 mbyrnepr2 marked this pull request as ready for review April 5, 2022 11:12
@Pierre-Sassoulas Pierre-Sassoulas merged commit ff5c7e3 into pylint-dev:main Apr 5, 2022
@mbyrnepr2 mbyrnepr2 deleted the overridden-final-method-documentation branch April 5, 2022 11:38
@Pierre-Sassoulas Pierre-Sassoulas removed this from the 2.14.0 milestone May 17, 2022
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.

3 participants