-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Enhanced support for @Property decorator in pyreverse #10057
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
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
9962519
Extract return types for property decorator
Julfried a707767
Update writer logic to also add properties to the diagram
Julfried a407835
Revert "Update writer logic to also add properties to the diagram"
Julfried 8ff2fdd
Implement a cleaner solution for extracting the return types of prope…
Julfried 22249a2
Add functional tests for property decorator
Julfried c424c3d
Merge into a single file
Julfried 5d558e9
Add newsfragment
Julfried e9a3af0
Fix newsfragment
Julfried 52880d3
Fix file name of newsframent
Julfried File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,3 @@ | ||
Enhanced support for @property decorator in pyreverse to correctly display return types of annotated properties when generating class diagrams. | ||
|
||
Closes #10057 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
7 changes: 7 additions & 0 deletions
7
tests/pyreverse/functional/class_diagrams/property_decorator/property_decorator.mmd
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,7 @@ | ||
classDiagram | ||
class AnnotatedPropertyTest { | ||
x : int | ||
} | ||
class NonAnnotatedPropertyTest { | ||
x | ||
} |
41 changes: 41 additions & 0 deletions
41
tests/pyreverse/functional/class_diagrams/property_decorator/property_decorator.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,41 @@ | ||
# property_decorator.py | ||
class AnnotatedPropertyTest: | ||
"""Test class for property decorators with annotated return type""" | ||
def __init__(self): | ||
self._x = 0 | ||
|
||
@property | ||
def x(self) -> int: | ||
"""This is a getter for x""" | ||
return self._x | ||
|
||
@x.setter | ||
def x(self, value): | ||
"""This is a setter for x""" | ||
self._x = value | ||
|
||
@x.deleter | ||
def x(self): | ||
"""This is a deleter for x""" | ||
del self._x | ||
|
||
|
||
class NonAnnotatedPropertyTest: | ||
"""Test class for property decorators without annotated return type""" | ||
def __init__(self): | ||
self._x = 0 | ||
|
||
@property | ||
def x(self): | ||
"""This is a getter for x""" | ||
return self._x | ||
|
||
@x.setter | ||
def x(self, value): | ||
"""This is a setter for x""" | ||
self._x = value | ||
|
||
@x.deleter | ||
def x(self): | ||
"""This is a deleter for x""" | ||
del self._x |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not that experienced, but do you think it is possible to infer the return type using astroid in case the property is not annotated? In essence add an else branch in here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In pylint we infer only and do not trust the typing (because there was no typing when pylint was created), so yes. Not sure what the philosophy in pyreverse should be. Probably that trusting the typing first then inferring makes sense (even if changing the philosophy in pylint would be humongous work and probably make pylint inconsistent).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this philosophy makes sense. Also assumed that pylint works that way, this is why I tried to come up with a solution using
node.infer_call_result()
however I always gotUNINFERABLE
. Probalby it is an error on my side, because I have not that much experience with astroid :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not surprised you're getting uninferable, property inferring in pylint is not very good. Making property inference in astroid better would help in both pyreverse and pylint but no one worked on it as of today.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes a lot of sense. So I guess for now relying on the annotations for the Properties is fine. Unfortunately I currently dont have the time to work on that myself. Maybe something to work on in a follow up PR