Skip to content

Fix Pyreverse: Aggregations aren't filtered according to filter mode (PUB_ONLY, etc.) #10379

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

Merged
merged 5 commits into from
May 21, 2025

Conversation

pavan-msys
Copy link
Contributor

Type of Changes

Type
βœ“ πŸ› Bug fix

Description

This PR fixes an issue in Pyreverse where class-level aggregations and associations were not being filtered according to the selected filter mode (PUB_ONLY, ALL, etc.).

Currently, even private or protected attributes result in relationships in the diagram, regardless of the filter setting. This fix ensures that relationships are only created for attributes that match the visibility filter.

Closes #10373

@pavan-msys pavan-msys requested a review from DudeNr33 as a code owner May 14, 2025 11:16
@Pierre-Sassoulas Pierre-Sassoulas added pyreverse Related to pyreverse component Bug πŸͺ² labels May 14, 2025
@Pierre-Sassoulas Pierre-Sassoulas added this to the 3.3.8 milestone May 14, 2025
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 working on this, do you mind addint test using the example givenin the issue please ?

This comment has been minimized.

@pavan-msys
Copy link
Contributor Author

Hi @Pierre-Sassoulas

I've added the tests using the example from the issue. Please let me know if there's anything else needed.

@Pierre-Sassoulas
Copy link
Member

Thank you and LGTM @DudeNr33 but I'm having second thought on my classification of the initial issue as a bug. Maybe it's voluntary to still show the link between things ? What do you think ?

This comment has been minimized.

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.

Thanks for the fix! @Pierre-Sassoulas I think it classifies as a bug. I can't say if the original author of the code did this on purpose, but the behavior is at least inconsistent: the name of the attribute is shown on the arrow, but the attribute is not listed in the class itself. I can't think of a reason why this should be the desired behavior.
Therefore it's best labelled as bugfix imo.

@pavan-msys looks like we do not yet have a test in our code base that checks the behavior for the other filter modes. Can you (for completeness) add additional functional checks that cover the other filter-mode options? You can specify configuration options for the functional tests by supplying a .rc file next to the test files with the same name. Thank you!

@Julfried
Copy link
Contributor

Hi @pavan-msys many thanks for working on this bug. I already started working on this issue a few weeks ago, but I only managed to add tests for all the filter options due to time constraints. If you want I could provide them for you :)

@pavan-msys
Copy link
Contributor Author

Hi @Julfried

Yes please, that would be very helpful. Thank you!

@Julfried
Copy link
Contributor

I think the easiest way is if you just get them from here: https://github.com/Julfried/pylint/tree/filter-agg

@pavan-msys
Copy link
Contributor Author

Hi @Julfried

Thanks for sharing the test cases. I have tested them manually, and all are working as expected. Could you please guide me on how to run the test cases using pytest?

@Julfried
Copy link
Contributor

Julfried commented May 19, 2025

If you have setup your environment according to this guide, you can launch the tests by running this command:

pytest test/pyreverse

@pavan-msys
Copy link
Contributor Author

Hi @DudeNr33

As per your suggestion, I have added additional functional checks to cover the other filter mode options.

Please let me know if any changes are required.

Copy link

codecov bot commented May 20, 2025

Codecov Report

All modified and coverable lines are covered by tests βœ…

Project coverage is 95.90%. Comparing base (6be8676) to head (bdfe107).
Report is 2 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff           @@
##             main   #10379   +/-   ##
=======================================
  Coverage   95.90%   95.90%           
=======================================
  Files         176      176           
  Lines       19126    19130    +4     
=======================================
+ Hits        18343    18347    +4     
  Misses        783      783           
Files with missing lines Coverage Ξ”
pylint/pyreverse/diagrams.py 93.61% <100.00%> (+0.13%) ⬆️
πŸš€ New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

πŸ€– According to the primer, this change has no effect on the checked open source code. πŸ€–πŸŽ‰

This comment was generated for commit bdfe107

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.

Thanks for adding the tests! Good to go now. πŸ™‚

@DudeNr33 DudeNr33 merged commit ed59632 into pylint-dev:main May 21, 2025
45 checks passed
pylint-backport-bot bot pushed a commit that referenced this pull request May 21, 2025
…(PUB_ONLY, etc.) (#10379)

* updated diagrams.py file

* added tests

* updated tests

* added test cases

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------

Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
(cherry picked from commit ed59632)
Pierre-Sassoulas pushed a commit that referenced this pull request May 21, 2025
…(PUB_ONLY, etc.) (#10379) (#10401)

* updated diagrams.py file

* added tests

* updated tests

* added test cases

* [pre-commit.ci] auto fixes from pre-commit.com hooks

for more information, see https://pre-commit.ci

---------


(cherry picked from commit ed59632)

Co-authored-by: pavan-msys <149513767+pavan-msys@users.noreply.github.com>
Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com>
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pyreverse: Aggregations aren't filtered according to filter mode (PUB_ONLY, etc.)
4 participants