-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Add type annotations to pyreverse #4551
Add type annotations to pyreverse #4551
Conversation
b485530
to
043c883
Compare
ead9a4c
to
caed9b0
Compare
caed9b0
to
4b35586
Compare
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.
Thank you for this changes :) I made some suggestions that could maybe simplify the code let me know if there is something that I did not see :)
pylint/pyreverse/inspector.py
Outdated
ann = getattr(node.parent, "annotation", None) | ||
values = {ann} if ann else set(node.infer()) |
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.
ann = getattr(node.parent, "annotation", None) | |
values = {ann} if ann else set(node.infer()) | |
values = {getattr(node.parent, "annotation", node.infer())} |
Would that work ?
pylint/pyreverse/inspector.py
Outdated
@@ -229,8 +230,17 @@ def handle_assignattr_type(node, parent): | |||
|
|||
handle instance_attrs_type | |||
""" | |||
ann = None |
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.
ann = None | |
ann = node.infer() |
I think doing that would simplify that following code.
@Pierre-Sassoulas thank you! I'll try that later today along with the further changes required to display variables a optional in the diagram. |
This would be fantastic! There is no Python UML generator that understands type annotations right now. This also lays the groundwork for the future generation of dependency arrows for class attributes. @mbyrnepr2 Did the requested changes work? |
@alexchandel the suggested changes work perfectly. Aiming to update the pr with those + Optional output this week. |
@DudeNr33 I don't know if you saw that MR but someone else if working on pyreverse in parallel :) Maybe you can rebase on it as soon as it's merged to avoid handling conflicts late. |
Thanks for the heads up! |
- Indicate the attribute is optional in the dot files by inspecting the default value. - Handle the Subscript type annotations - Refactor & move the logic to obtain annotations to utils.py as re-usable functions - Add unittests for the added functions
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.
Thank you for going back to this. I think there's should be a handling for the inference fail that can happen with infer() call. I don't have an example of code where there inference would fail, maybe we can patch NodeNg
so it raises one in order to be able to test ?
pylint/pyreverse/inspector.py
Outdated
@@ -218,7 +218,8 @@ def visit_assignname(self, node): | |||
self.visit_module(frame) | |||
|
|||
current = frame.locals_type[node.name] | |||
values = set(node.infer()) | |||
ann = utils.get_annotation(node) | |||
values = {ann} if ann else set(node.infer()) |
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.
Here there can be an inference fail, we should catch it (like line 238). Btw it look like the code is similar, maybe we can create a function for it ?
pylint/pyreverse/utils.py
Outdated
else: | ||
return ann | ||
|
||
default, *_ = node.infer() |
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.
There can be an InferenceError
raised here.
- Add a try/except to deal with a possible InferenceError when using NodeNG.infer method - Create a function in utils and so remove repeated logic in inspector.py - Add unittests to check the InferenceError logic - Adjust the types in function input
Thanks again @Pierre-Sassoulas I agree with all the above. I've aimed to address the points in 194e6d3. I could easily have missed something though so happy to look again if something doesn't look right. |
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.
Thank you for all the work that went into this !
Closes #1548
Steps
doc/whatsnew/<current release.rst>
.Description
demo.py:
development branch:

master branch:

Type of Changes
Related Issue