-
Notifications
You must be signed in to change notification settings - Fork 139
fix: update mpwrite and mpread methods to enforce 'LIB' as the only valid lib argument #4123
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
Conversation
…alid lib argument
Reviewer's GuideEnforce 'LIB' as the sole valid library argument by updating mpwrite and mpread implementations to use default lib='LIB' with proper validation and file handling, remove legacy behaviors, and update tests to reflect the new API. Class diagram for updated mpwrite and mpread methodsclassDiagram
class MapdlExtended {
+mpwrite(fname, ext, lib="LIB", mat, download_file, progress_bar, **kwargs)
+mpread(fname, ext, lib="LIB", **kwargs)
}
MapdlExtended --|> _MapdlCore
class _MapdlCore {
+mpwrite(fname, ext, lib, mat, **kwargs)
+mpread(fname, ext, lib, **kwargs)
}
Flow diagram for mpread 'lib' argument validationflowchart TD
A[Call mpread with lib argument] --> B{Is lib == 'LIB'?}
B -- No --> C[Raise MapdlRuntimeError]
B -- Yes --> D[Proceed with file handling and input]
File-Level Changes
Assessment against linked issues
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.
Pull Request Overview
This PR updates the mpwrite
and mpread
methods to enforce 'LIB' as the only valid value for the lib
argument, addressing issue #3364. The changes remove the previous workaround that bypassed mpread
with input
and updates the implementation to properly validate the lib
parameter.
- Replaces the previous
NotImplementedError
with proper validation that only accepts 'LIB' for thelib
argument - Updates
mpread
to use internal file handling methods instead of bypassing withinput
- Sets 'LIB' as the default value for the
lib
parameter in both methods
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
File | Description |
---|---|
src/ansys/mapdl/core/mapdl_extended.py |
Updates mpwrite and mpread methods to enforce 'LIB' validation and use proper file handling |
tests/test_mapdl.py |
Adds comprehensive test coverage for the new functionality and removes tests for the old error behavior |
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 and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `src/ansys/mapdl/core/mapdl_extended.py:369` </location>
<code_context>
- "The option 'lib' is not supported by the MAPDL gRPC server."
- )
+ def mpread(self, fname="", ext="", lib="LIB", **kwargs):
+ if lib != "LIB":
+ raise MapdlRuntimeError("The 'lib' argument only support 'LIB' value.")
- fname_ = fname + "." + ext
</code_context>
<issue_to_address>
The error message in MapdlRuntimeError contains a grammatical issue.
Please update the message to: 'The 'lib' argument only supports the 'LIB' value.'
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if lib != "LIB":
raise MapdlRuntimeError("The 'lib' argument only support 'LIB' value.")
=======
if lib != "LIB":
raise MapdlRuntimeError("The 'lib' argument only supports the 'LIB' value.")
>>>>>>> REPLACE
</suggested_fix>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Co-authored-by: sourcery-ai[bot] <58596630+sourcery-ai[bot]@users.noreply.github.com>
…apdl into fix/mpread-and-mpwrite
Pinging @mikerife for advice. So far, we have set as default to use What do you think? |
…ror handling for unsupported lib argument
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4123 +/- ##
==========================================
- Coverage 91.35% 91.28% -0.08%
==========================================
Files 189 189
Lines 15650 15700 +50
==========================================
+ Hits 14297 14331 +34
- Misses 1353 1369 +16 🚀 New features to boost your workflow:
|
@pyansys-ci-bot 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.
Description
As the title.
Issue linked
Close #3364
Checklist
draft
if it is not ready to be reviewed yet.feat: adding new MAPDL command
)Summary by Sourcery
Enforce "LIB" as the sole acceptable lib argument in mpwrite and mpread, updating defaults, error handling, and internal file-loading logic, and adjust tests to validate the new behavior.
Bug Fixes:
Enhancements:
Tests: