-
Notifications
You must be signed in to change notification settings - Fork 196
FEAT: add dir as property #6716
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
base: main
Are you sure you want to change the base?
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6716 +/- ##
==========================================
+ Coverage 81.04% 83.25% +2.20%
==========================================
Files 245 246 +1
Lines 77272 77402 +130
==========================================
+ Hits 62623 64438 +1815
+ Misses 14649 12964 -1685 🚀 New features to boost your workflow:
|
# Conflicts: # src/ansys/aedt/core/generic/numbers.py
# Conflicts: # tests/system/general/test_01_general_methods.py
After doing some research, I see no objection to your proposal, as I could not find any resource discouraging this type of implementation. I think using the builtin I do understand the usecase for that new property and the usability improvement. Same goes for the All in all, the changes do look good to me (nice that tests are provided as well)! |
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.
Thanks for opening that PR @Alberto-DM
While I do like the idea of having some mixin that could share behavior over all classes, I'm not really fan of the dir
property. I might be wrong, but I feel like this property brings:
- risk of confusion for people who are familiar with
dir()
, this "breaks" implicit convention of the language; - little additional value when the risk of confusion are non negligible.
Also, I think we should focus on improving the type hints provided through the code base. This would really help a lot with autocompletion in modern IDEs. When thinking about notebook, isn't the use of already enough to get the attributes ? If it's because the list starts with private / protected attributes and methods, we could just rewrite__dir__
by sorting the elements and placing the one with_
at the end of the list.
Other though: renaming into public_attrs
would avoid confusion.
Open to discussion :)
@SMoraisAnsys I understand your concerns. Regarding the utility of having dir as a property: I personally use I also really like the idea of improving autocompletion by reordering the @ecoussoux-ansys yes, implementing |
# Conflicts: # src/ansys/aedt/core/visualization/post/post_circuit.py # src/ansys/aedt/core/visualization/post/solution_data.py
Following the suggestions from @SMoraisAnsys I renamed the |
Description
This PR introduces a
DirMixin
and a centralPyAedtBase
class to simplify interactive exploration and provide a unified foundation for all project classes.DirMixin
adds a.public_dir
property for instances to quickly inspect available attributes and methods, filtering out private/internal names.DirMixin
redefines the__dir__
method to reorder the output in order to improve IDEs autocompletion. The public attributes are shown first, followed by the private ones (starting with_
).PyAedtBase
inherits fromDirMixin
and serves as a central base class for all other classes, making it easy to add additional mixins or shared functionality in the future.Example
Prior to this feature the method exploration required to type back and forth in the command line:
Now we can explore the methods and properties of an object by simply doing this:
Benefits