Skip to content

Fix pyreverse type hinting for class methods #5881

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

Merged
merged 4 commits into from
Mar 9, 2022
Merged

Fix pyreverse type hinting for class methods #5881

merged 4 commits into from
Mar 9, 2022

Conversation

teobouvard
Copy link
Contributor

This commit fixes the alignment of arguments and their type annotations
in pyreverse printer output.

  • 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>.
    Not a feature, is it an important bug fix ?
  • Write a good description on what the PR does.

Type of Changes

Type
🐛 Bug fix

Description

Until now, the following class

class Example:
    def method(self, arg: int):
        pass
    
    @classmethod
    def class_method(cls, arg: int):
        pass
    
    @staticmethod
    def static_method(arg: int):
        pass

would generate the following PlantUML diagram when running pyreverse -o puml example.py

@startuml example
class Example {
  class_method(cls: int, arg)
  method(arg: int)
  static_method(arg)
}
@enduml

This output displays wrong type hints because the alignment of arguments and their type annotations is based on the first argument having name self. This is not the case for class methods, where the first argument is usually cls and static methods where the first argument is not related to the class.

This commit fixes the issue by performing the alignment using the method type rather than the parameters names. This generates the correct class diagram shown below.

@startuml example
class Example {
  class_method(arg: int)
  method(arg: int)
  static_method(arg: int)
}
@enduml

This commit fixes the alignment of arguments and their type annotations
in pyreverse printer output. It does so by checking for the type of the
current function rather than the name of the first argument.
This allows class methods having a non-standard first argument
(different from "self" or "cls") to be correctly serialized in class
diagrams.
@coveralls
Copy link

coveralls commented Mar 8, 2022

Pull Request Test Coverage Report for Build 1953999813

  • 5 of 5 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.007%) to 94.011%

Totals Coverage Status
Change from base Build 1952197418: 0.007%
Covered Lines: 14944
Relevant Lines: 15896

💛 - Coveralls

@teobouvard
Copy link
Contributor Author

I'm going to try to cover the else branch to prevent a decrease of code coverage

@teobouvard teobouvard marked this pull request as draft March 8, 2022 18:56
@Pierre-Sassoulas Pierre-Sassoulas added Bug 🪲 pyreverse Related to pyreverse component labels Mar 8, 2022
@Pierre-Sassoulas Pierre-Sassoulas added this to the 2.14.0 milestone Mar 8, 2022
According to astroid docs, this happens for builtin functions
implemented in C. In this case, we return an empty argument list.
@teobouvard teobouvard marked this pull request as ready for review March 8, 2022 22:22
@DanielNoord DanielNoord requested a review from DudeNr33 March 9, 2022 08:04
@DanielNoord
Copy link
Collaborator

@Pierre-Sassoulas, did you change the coveralls setting to disallow decreasing coverage PRs? The CI seems to have passed on that commit that did not have full coverage.

@teobouvard Thanks for the contribution! I have requested a review from @DudeNr33 as they are the pyreverse expert I believe.

@Pierre-Sassoulas
Copy link
Member

Pierre-Sassoulas commented Mar 9, 2022

did you change the coveralls setting to disallow decreasing coverage PRs?

No, I think it can be problematic, for example in #5874 (comment) we removed code and the coverage decreased. I don't want to (have to) deal with this kind of false positive. I think the easier thing to do would be to check the coverage before approving.

annotations = dict(zip(arguments, method.args.annotations[1:]))
if method.args.args is None:
return []

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice cleanup and lowering the indentation level!

Copy link
Collaborator

@DudeNr33 DudeNr33 left a comment

Choose a reason for hiding this comment

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

Nice catch, thank you for the fix. Looks good to me, well tested with both relevant cases added to the example test data. 👍

Only thing I noticed: this PR was set to milestone 2.14.0, while the changelog has the change listed for 2.13.0.

@Pierre-Sassoulas Pierre-Sassoulas modified the milestones: 2.14.0, 2.13.0 Mar 9, 2022
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 for the quick review @DudeNr33 👍 Let's put it in 2.13 then !

@Pierre-Sassoulas Pierre-Sassoulas merged commit 1ebdc8c into pylint-dev:main Mar 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug 🪲 pyreverse Related to pyreverse component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants