-
Notifications
You must be signed in to change notification settings - Fork 132
exposing a reconnection method #3933
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
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
Reviewer's GuideRefactored the gRPC connection setup by extracting the multi_connect logic into a private helper and exposing a new public reconnect_to_mapdl API that can re-establish dropped connections with an optional timeout, replacing the inline connection code in the constructor. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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 @germa89 - I've reviewed your changes - here's some feedback:
- Replace
if not timeout:
with an explicitif timeout is None:
check so that a zero timeout can be handled correctly. - Add explicit
-> None
return type annotations to both_connect_to_mapdl
andreconnect_to_mapdl
for consistency with the existing type hints. - Consider returning a status flag or raising a custom exception in
reconnect_to_mapdl
to clearly signal success or failure to callers instead of implicitNone
.
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Testing: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
if not timeout: | ||
timeout = self._timeout |
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.
suggestion (bug_risk): Use timeout is None
instead of falsy check
This prevents valid falsy values (e.g., 0) from being overridden by self._timeout
.
if not timeout: | |
timeout = self._timeout | |
if timeout is None: | |
timeout = self._timeout |
Description
Expose a reconnection method
Issue linked
NA
Checklist
draft
if it is not ready to be reviewed yet.feat: adding new MAPDL command
)Summary by Sourcery
Extract connection logic from init and provide a dedicated reconnect_to_mapdl() API to allow re-establishing a stopped or crashed MAPDL gRPC session without relaunching the instance.
New Features:
Enhancements: