Enhanced support for @Property decorator in pyreverse #10057

Julfried · 2024-10-30T14:53:29Z

Type of Changes

	Type
✓	✨ New feature

Description

Closes #9212

Closes #7644

Fixed by properly extracting the return types when handling class attributes. Implementation is much cleaner than before.

This reverts commit a707767. Revert "Extract return types for property decorator" This reverts commit 9962519.

…rties

codecov · 2025-03-02T21:59:07Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.86%. Comparing base (1e08448) to head (52880d3).
Report is 27 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main   #10057   +/-   ##
=======================================
  Coverage   95.86%   95.86%           
=======================================
  Files         175      175           
  Lines       19069    19072    +3     
=======================================
+ Hits        18281    18284    +3     
  Misses        788      788

Files with missing lines	Coverage Δ
pylint/pyreverse/diagrams.py	`93.47% <100.00%> (+0.14%)`	⬆️

... and 2 files with indirect coverage changes

🚀 New features to boost your workflow:

❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

github-actions · 2025-03-02T22:14:22Z

🤖 According to the primer, this change has no effect on the checked open source code. 🤖🎉

This comment was generated for commit 8ff2fdd

Julfried · 2025-03-03T21:02:02Z

@Pierre-Sassoulas I cleaned up my previous solution and have it actually working, when there is a property that has a return type annotated. However if there is no return type annotated it does not work, so the current test structure in tests/data isn`t ideal for testing this. Changing the structure however would require updating all the resulting diagrams. Not sure if I should do that?

Julfried · 2025-03-03T21:02:24Z

pylint/pyreverse/diagrams.py

+            ) and decorated_with_property(associated_nodes):
+                if associated_nodes.returns:
+                    type_annotation = get_annotation_label(associated_nodes.returns)
+                    node_name = f"{node_name} : {type_annotation}"


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

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

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 got UNINFERABLE. Probalby it is an error on my side, because I have not that much experience with astroid :)

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.

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

Julfried · 2025-03-06T16:15:58Z

I guess we could fix coverage by annotating the the properties under tests/data/property_pattern.py. However this would mean I need to update all the test files. Maybe updating this files should be automated somehow?

DudeNr33 · 2025-03-06T17:18:02Z

I guess we could fix coverage by annotating the the properties under tests/data/property_pattern.py. However this would mean I need to update all the test files. Maybe updating this files should be automated somehow?

You can try using the functional tests for pyreverse. You can add new files and expected outputs below this folder: https://github.com/pylint-dev/pylint/tree/main/tests/pyreverse/functional/class_diagrams

They are picked up automatically, and the output generated by pyreverse is compared to the reference output.
If you need special command line options, or want to control what output format to create, you can create an .rc file to configure pyreverse like this: https://github.com/pylint-dev/pylint/blob/main/tests/pyreverse/functional/class_diagrams/colorized_output/colorized.rc

Julfried · 2025-03-06T18:34:07Z

Thats nice, I did not know that, Then I will use this for testing :)

Julfried · 2025-03-07T18:45:27Z

I think the pypy error is not related to my changes

DudeNr33

I feel like putting both functional tests (annotated and not annotated) into a single file fits more with the current 'one functional test file per feature/behavior', but that is personal preference. 🙂 Otherwise looks good, thanks for the fix. Let's see if adding a newsfragment also fixes the PyPy problem, as simply re-running the job did not help.

Julfried · 2025-03-10T09:49:33Z

Added a newsfragment and merged it into 1 file. Pypy action is still failing though

Pierre-Sassoulas · 2025-03-10T10:56:38Z

Don't bother with pypi3.10 it's failing on main too :)

DudeNr33

Thanks again for fixing this issue!

Julfried requested a review from DudeNr33 as a code owner October 30, 2024 14:53

This comment has been minimized.

Sign in to view

Pierre-Sassoulas changed the title ~~Property decorator~~ Enhanced support for @Property decorator in pyreverse Oct 31, 2024

Julfried marked this pull request as draft December 14, 2024 23:58

Julfried added 2 commits March 2, 2025 19:18

Extract return types for property decorator

9962519

Update writer logic to also add properties to the diagram

a707767

Julfried force-pushed the property-decorator branch from a380c46 to a707767 Compare March 2, 2025 18:23

This comment has been minimized.

Sign in to view

Julfried added 2 commits March 2, 2025 22:14

Revert "Update writer logic to also add properties to the diagram"

a407835

This reverts commit a707767. Revert "Extract return types for property decorator" This reverts commit 9962519.

Implement a cleaner solution for extracting the return types of prope…

8ff2fdd

…rties

Julfried commented Mar 3, 2025

View reviewed changes

Pierre-Sassoulas added pyreverse Related to pyreverse component Enhancement ✨ Improvement to a component Needs decision 🔒 Needs a decision before implemention or rejection labels Mar 3, 2025

Add functional tests for property decorator

22249a2

Julfried marked this pull request as ready for review March 7, 2025 18:45

Pierre-Sassoulas added this to the 4.0.0 milestone Mar 9, 2025

DudeNr33 requested changes Mar 9, 2025

View reviewed changes

Julfried added 4 commits March 10, 2025 10:11

Merge into a single file

c424c3d

Add newsfragment

5d558e9

Fix newsfragment

e9a3af0

Fix file name of newsframent

52880d3

DudeNr33 approved these changes Mar 10, 2025

View reviewed changes

DudeNr33 merged commit 512c8be into pylint-dev:main Mar 10, 2025
43 of 44 checks passed

Julfried deleted the property-decorator branch March 10, 2025 20:26

Uh oh!

Enhanced support for @Property decorator in pyreverse #10057

Enhanced support for @Property decorator in pyreverse #10057

Uh oh!

Conversation

Julfried commented Oct 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Type of Changes

Description

Uh oh!

This comment has been minimized.

This comment has been minimized.

codecov bot commented Mar 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

github-actions bot commented Mar 2, 2025

Uh oh!

Julfried commented Mar 3, 2025

Uh oh!

Julfried Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas Mar 3, 2025

Choose a reason for hiding this comment

Uh oh!

Julfried Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

Pierre-Sassoulas Mar 5, 2025

Choose a reason for hiding this comment

Uh oh!

Julfried Mar 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Julfried commented Mar 6, 2025

Uh oh!

DudeNr33 commented Mar 6, 2025

Uh oh!

Julfried commented Mar 6, 2025

Uh oh!

Julfried commented Mar 7, 2025

Uh oh!

DudeNr33 left a comment

Choose a reason for hiding this comment

Uh oh!

Julfried commented Mar 10, 2025

Uh oh!

Pierre-Sassoulas commented Mar 10, 2025

Uh oh!

DudeNr33 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Julfried commented Oct 30, 2024 •

edited

Loading

codecov bot commented Mar 2, 2025 •

edited

Loading

Julfried Mar 6, 2025 •

edited

Loading