refactor: move methods from MapdlGrpc class to core and extended - #4108
Merged
Conversation
Contributor
|
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
Contributor
Reviewer's GuideThis refactor centralizes file handling, session management, and screenshot utilities into MapdlCore, moves command-specific wrappers into MapdlExtended, and cleans up redundant implementations in MapdlGrpc. Class diagram for refactored MapdlCore, MapdlExtended, and MapdlGrpcclassDiagram
class MapdlGrpc {
- Removed: list_files()
- Removed: _get_file_path()
- Removed: _get_file_name()
- Removed: file()
- Removed: igesin()
- Removed: satin()
- Removed: cat5in()
- Removed: parain()
- Removed: screenshot()
- Removed: _create_session()
- Removed: _session_id
- Removed: _check_session_id()
- Removed: _get_mapdl_session_id()
}
class MapdlCore {
+ list_files(refresh_cache: bool = True) : List[str]
+ _get_file_path(fname: str, progress_bar: bool = False) : str
+ _get_file_name(fname, ext, default_extension) : str
+ screenshot(savefig: Optional[str] = None) : str
+ _create_session()
+ _session_id
+ _check_session_id()
+ _get_mapdl_session_id()
}
class MapdlExtended {
+ file(fname: str = "", ext: str = "", **kwargs) : str
+ igesin(fname, ext = "", **kwargs)
+ satin(name, extension = "", path = "", entity = "", fmt = "", nocl = "", noan = "", **kwargs)
+ cat5in(name, extension = "", path = "", entity = "", fmt = "", nocl = "", noan = "", **kwargs)
+ parain(name, extension = "", path = "", entity = "", fmt = "", scale = "", **kwargs)
}
MapdlGrpc <|-- MapdlCore
MapdlCore <|-- MapdlExtended
Class diagram for file handling and session management centralizationclassDiagram
class MapdlCore {
+ list_files()
+ _get_file_path()
+ _get_file_name()
+ screenshot()
+ _create_session()
+ _session_id
+ _check_session_id()
+ _get_mapdl_session_id()
}
class MapdlExtended {
+ file()
+ igesin()
+ satin()
+ cat5in()
+ parain()
}
MapdlCore <|-- MapdlExtended
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the PyMAPDL codebase by moving methods from the MapdlGrpc class to more appropriate locations in the core and extended modules. The changes improve code organization by distributing functionality according to architectural boundaries.
- Moved several file handling and session management methods from
mapdl_grpc.pytomapdl_core.py - Moved CAD file import methods to
mapdl_extended.pyfor better modularity - Updated the
filemethod implementation to handle different default extensions correctly
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
src/ansys/mapdl/core/mapdl_grpc.py |
Removed methods and imports that were moved to core/extended modules |
src/ansys/mapdl/core/mapdl_core.py |
Added core functionality methods including file handling, session management, and screenshot capability |
src/ansys/mapdl/core/mapdl_extended.py |
Added CAD file import methods and updated file method with new logic |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @germa89 - I've reviewed your changes and found some issues that need to be addressed.
Blocking issues:
- Variable 'path_' is referenced but not defined. (link)
General comments:
- In mapdl_extended.py the file() override unpacks only file_, ext_, _ but then uses path_ which is never defined—adjust the unpacking to include path_ correctly.
- You’ve changed the default extension in mapdl_extended.file from "rst" to "cdb"—please confirm this aligns with existing use cases to avoid unintended breakage.
- The file‐handling helpers (_get_file_path, _get_file_name, list_files, screenshot) are duplicated across Core and Extended—consider extracting them into a shared mixin or utility module to reduce code drift.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In mapdl_extended.py the file() override unpacks only file_, ext_, _ but then uses path_ which is never defined—adjust the unpacking to include path_ correctly.
- You’ve changed the default extension in mapdl_extended.file from "rst" to "cdb"—please confirm this aligns with existing use cases to avoid unintended breakage.
- The file‐handling helpers (_get_file_path, _get_file_name, list_files, screenshot) are duplicated across Core and Extended—consider extracting them into a shared mixin or utility module to reduce code drift.
## Individual Comments
### Comment 1
<location> `src/ansys/mapdl/core/mapdl_extended.py:109` </location>
<code_context>
- fname = self._get_file_name(fname, ext, "rst")
+ fname = self._get_file_name(fname, ext, "cdb")
fname = self._get_file_path(fname, kwargs.get("progress_bar", False))
file_, ext_, _ = self._decompose_fname(fname)
- return self._file(file_, ext_, **kwargs)
+ if self._local:
+ return self._file(filename=path_ / file_, extension=ext_, **kwargs)
+ else:
</code_context>
<issue_to_address>
Variable 'path_' is referenced but not defined.
'path_' is not defined in this context. Unpack it from '_decompose_fname' or update the variable name to match the unpacked values to avoid a NameError.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov ReportAttention: Patch coverage is
❌ Your patch status has failed because the patch coverage (85.63%) is below the target coverage (90.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #4108 +/- ##
==========================================
- Coverage 91.39% 91.30% -0.09%
==========================================
Files 189 189
Lines 15667 15649 -18
==========================================
- Hits 14319 14289 -30
- Misses 1348 1360 +12 🚀 New features to boost your workflow:
|
Collaborator
Author
|
Since it is a refactoring, let's not check the coverage. @pyansys-ci-bot LGTM. |
germa89
enabled auto-merge (squash)
July 21, 2025 18:46
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
As the title.
Issue linked
Close #4102
Checklist
draftif it is not ready to be reviewed yet.feat: adding new MAPDL command)Summary by Sourcery
Refactor MapdlGrpc by relocating file handling, session management, screenshot, and import command wrapper methods into MapdlCore and MapdlExtended to streamline class responsibilities and close issue #4102
Enhancements: