-
Notifications
You must be signed in to change notification settings - Fork 128
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
FEAT: Circuit current probe #5352
Conversation
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5352 +/- ##
==========================================
+ Coverage 84.38% 84.40% +0.01%
==========================================
Files 140 140
Lines 58635 58645 +10
==========================================
+ Hits 49479 49499 +20
+ Misses 9156 9146 -10 |
Hi @boyang2022 , you missed to test when you do not define the location. Thank you for the pull request! |
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.
Hey @boyang2022 , thanks for the contribution !
This changes seem to be copy pasted from create_voltage_probe
(which isn't a problem!). Could you refactor that to have a single method with an additional argument to distinguish between voltage and current and which would contain the logic, e.g. __create_probe
?
That way, both create_voltage_probe
and create_current_probe
could call that method behind the scene and that would help us to maintain both at the same time.
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.
LGTM
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.
Could you please revert the parameter name
as an optional input ? This would introduce a breaking change for users and from the changes you are doing, I don't think it is required.
@Samuelopez-ansys If you think that changing name as a required value is a good thing, feel free to discard my review.
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.
LGTM
No description provided.