refactor: using Path method for joining paths. - #4101
Conversation
|
Thanks for opening a Pull Request. If you want to perform a review write a comment saying: @ansys-reviewer-bot review |
Reviewer's GuideThis PR refactors the codebase to use pathlib.Path’s '/' operator for path joining and file operations, updates the directory property to return a PurePath, improves typing in the supress_logging decorator, and initializes the hostname attribute in MapdlGRPC. Class diagram for updated directory handling and supress_logging decoratorclassDiagram
class MapdlCore {
+directory: pathlib.PurePath
+_wrap_directory(path: str) pathlib.PurePath
}
class MapdlGRPC {
+_hostname: Optional[str]
+directory: pathlib.PurePath
}
class MapdlExtended {
+directory: pathlib.PurePath
}
class Parameters {
+_mapdl: MapdlCore
}
class supress_logging {
<<function>>
+__call__(func: Callable[P, R]) -> Callable[P, R]
}
MapdlGRPC --|> MapdlCore
Parameters --> MapdlCore : uses _mapdl
MapdlExtended --|> MapdlCore
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
@pyansys-ci-bot LGTM |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors path handling to use the / operator on pathlib PurePath objects instead of os.path.join for joining paths across tests and core modules.
- Replace
os.path.join(...)withpathlib-style division in tests and source code - Change
directoryproperty return type fromstrtopathlib.PurePath - Update decorator typing in
misc.pyto useParamSpecandTypeVar
Reviewed Changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_plotting.py | Use mapdl.directory / last_png for constructing PNG path |
| tests/test_misc.py | Replace os.path.join with mapdl.directory / file_ |
| tests/test_mapdl.py | Refactor join logic to use pathlib division for file paths |
| tests/test_krylov.py | Use mapdl.directory / ... when building full file path |
| tests/test_grpc.py | Swap os.path.join for temp_dir / ... in download tests |
| src/ansys/mapdl/core/parameters.py | Convert local filename to str(pathlib.PurePath / filename) |
| src/ansys/mapdl/core/misc.py | Add ParamSpec/TypeVar imports and type hints for decorator |
| src/ansys/mapdl/core/mapdl_grpc.py | Replace os.path.join with self.directory / ... and use glob on paths |
| src/ansys/mapdl/core/mapdl_extended.py | Use self.directory / filename for listing |
| src/ansys/mapdl/core/mapdl_core.py | Change directory return type to PurePath and update joins |
Comments suppressed due to low confidence (4)
src/ansys/mapdl/core/mapdl_core.py:526
- Changing the
directoryproperty return type fromstrtopathlib.PurePathis a breaking API change. Consider preserving backward compatibility or clearly documenting this impact.
def directory(self) -> pathlib.PurePath:
src/ansys/mapdl/core/mapdl_grpc.py:2128
- In
_get_file_path, you now assign aPathbut the method signature indicates astrreturn. Consider casting tostr(...)or updating the return annotation to avoid type inconsistencies.
filename = self.directory / fname
src/ansys/mapdl/core/mapdl_grpc.py:2534
- [nitpick] Passing a
pathlib.Pathdirectly intoglob.globmay rely on implicit conversion. For clarity and consistency, wrap the path instr(...).
)
tests/test_plotting.py:1024
- [nitpick] This makes
last_pngaPathwhen local but leaves it astrfor remote. Consider wrapping instr(...)to keep the returned type consistent across both branches.
last_png = mapdl.directory / last_png
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:
- glob.glob expects a string pattern, not a Path object. (link)
General comments:
- The directory property now returns a PurePath, but many downstream operations (like
.exists()or.resolve()) aren’t available on PurePath—consider returning a full pathlib.Path instead to preserve filesystem methods. - There’s still a mix of os.path and pathlib calls scattered throughout—refactor to use Path methods exclusively (e.g. Path.glob, Path.is_file, Path.exists) for consistency and clarity.
- You’ve introduced ParamSpec in the
supress_loggingdecorator which requires Python 3.10+; if you need to maintain earlier Python support, consider importing ParamSpec from typing_extensions or providing a fallback.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The directory property now returns a PurePath, but many downstream operations (like `.exists()` or `.resolve()`) aren’t available on PurePath—consider returning a full pathlib.Path instead to preserve filesystem methods.
- There’s still a mix of os.path and pathlib calls scattered throughout—refactor to use Path methods exclusively (e.g. Path.glob, Path.is_file, Path.exists) for consistency and clarity.
- You’ve introduced ParamSpec in the `supress_logging` decorator which requires Python 3.10+; if you need to maintain earlier Python support, consider importing ParamSpec from typing_extensions or providing a fallback.
## Individual Comments
### Comment 1
<location> `src/ansys/mapdl/core/mapdl_core.py:2535` </location>
<code_context>
def _screenshot_path(self):
"""Return last filename based on the current jobname"""
- filenames = glob.glob(os.path.join(self.directory, f"{self.jobname}*.png"))
+ filenames = glob.glob(self.directory / f"{self.jobname}*.png")
filenames.sort()
return filenames[-1]
</code_context>
<issue_to_address>
glob.glob expects a string pattern, not a Path object.
Convert the Path to a string with str() before passing it to glob.glob to avoid a TypeError.
</issue_to_address>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>
…-path-methods-to-join-paths
…-path-methods-to-join-paths
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4101 +/- ##
==========================================
+ Coverage 91.73% 91.82% +0.08%
==========================================
Files 187 187
Lines 15042 15047 +5
==========================================
+ Hits 13799 13817 +18
+ Misses 1243 1230 -13 🚀 New features to boost your workflow:
|
…pdlCommandExtended class test: enhance test_inquire_exist to use temporary files and validate existence checks

Description
As the title. Refactor to use the
/for joining paths.Issue linked
NA but using #4079
Checklist
draftif it is not ready to be reviewed yet.feat: adding new MAPDL command)Summary by Sourcery
Refactor path handling to use pathlib.Path and the
/operator across the codebase, update thedirectoryproperty type and tests accordingly, and enhance decorator typing.Enhancements:
/operatordirectoryproperty to return a pathlib.PurePath instead of a stringsupress_loggingdecoratorTests:
directoryusage and adjust assertions accordingly