Refactor depth filtering logic

[Julfried]

Type of Changes

	Type
✓	🔨 Refactoring

Description

Refactored the depth filtering logic, as discussed in #10077. Implementation should be much cleaner now :)

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 95.86%. Comparing base (05e83c4) to head (0e629d9).
Report is 31 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #10200      +/-   ##
==========================================
+ Coverage   95.84%   95.86%   +0.02%     
==========================================
  Files         175      175              
  Lines       19056    19053       -3     
==========================================
+ Hits        18264    18265       +1     
+ Misses        792      788       -4     

Files with missing lines	Coverage Δ
pylint/pyreverse/diadefslib.py	99.30% <100.00%> (+0.10%)	⬆️
pylint/pyreverse/main.py	91.66% <100.00%> (ø)
pylint/pyreverse/writer.py	100.00% <ø> (+0.84%)	⬆️
pylint/typing.py	100.00% <100.00%> (ø)

... and 10 files with indirect coverage changes

[Pierre-Sassoulas]

Thank you for the merge request ! LGTM, some nits:

[Julfried]

Thanks for the review, your suggestions make sense.

One thing I noticed when playing around with it. If you call pyreverse for a subpackage and apply a depth limit. For example:

pyreverse --max-depth 1 package.subpackage 

The diagram is correctly generated and starts only at subpackage. However the depth limit is still calculated with respect to the root package. I think it would be more intuitive to calculate depth relative to subpackage in this case. What do you think?

[Julfried]

However I am unsure how to achive this given that pyreverse lets you specify multiple packages to analyse

[Pierre-Sassoulas]

Yeah hard to reconcile all the arguments. Maybe it's easier to add a warning like "You asked for depth=1 but linked package.subpackage which require depth=2 to be shown as pyreverse count depth from the root, did you meant package or depth=3 ? "

[DudeNr33]

Thank you for the refactor! I agree with Pierre, I would go for a warning right now for the case you mentioned.

[Julfried]

Thanks for the review and for the suggestion regarding the warning! For properly implementing this warning I would need to implement this in main.py so that I dont have to gather the arguments passed. Maybe it makes more sense to just document this behavior instead?

[DudeNr33]

Yes, maybe adding a short notice in the help text of the option that explains how the counting works/where it starts would be the best solution for now.

[Pierre-Sassoulas]

From experience no one ever read the documentation, so I would favor a warning (unless it's really unreasonable to do it of course).

[Julfried]

I agree with Pierre that documentation is hardly read, so a warning would be much more effective in guiding users. However, to properly detect this edge case, I would need access to the original specified by the user (self.args in main.py) inside DiadefsHandler. Right now, I don’t see a straightforward way to pass this information without modifying the argument flow, but I’ll think about possible solutions. If anyone has suggestions on a clean way to achieve this, I’d be happy to discuss!

[Julfried]

Sorry for the delay, had to do prioritize some other stuff. I changed the depth logic to always choose the deepest packge (in other words the leaf nodes of the specified package tree). If a user specifies nested package names, a warning is now emitted to prevent confusion. I believe this approach is more intuitive and aligns with user expectations. Let me know what you think—I'll update the documentation accordingly if you approve :)

[Pierre-Sassoulas]

Thank you ! It's getting kinda big to review, we might want to do the refactor moving code from test_writer to test_diadefs in a prior refactor PR to ease this one.

My only actual request changes is on the logging that should be a warning :)

[github-actions]

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

This comment was generated for commit 0e629d9

[Julfried]

Thanks Pierre, I`ve applied your suggestions and fixed the tests. Regarding the warning, Ruff forced me to set a stacklevel for warnings.warn(). Based on my understanding, stacklevel=2 is appropriate for libraries as it makes the warning point to the calling code rather than our implementation details. Let me know what you think on that one.

I agree that the PR is getting quite large. However, since this PR moves all the filtering logic from writer.py to diadefslib.py, I am unsure if it is a good idea to move the tests in a seperate PR, prior to this one. How about updating docs in a follow up PR, since this is the last thing missing I guess.