refactor: modernize imports and use pathlib APIs - #381
Merged
jan-janssen merged 6 commits intoJul 29, 2026
Conversation
Prefer pathlib.Path over os.path/open in compatibility and output modules, modernize typing imports (PEP 604, collections.abc), and simplify redundant else branches. black --check and unittest suite pass locally (153 tests).
for more information, see https://pre-commit.ci
Contributor
📝 WalkthroughWalkthroughThe PR standardizes filesystem operations on ChangesParser modernization
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/lammpsparser/units.py (1)
240-244: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
stackleveltowarnings.warn.Without
stacklevel=2, the warning will point at this internal line rather than the caller's code, making it harder for users to locate the offending call.♻️ Proposed fix
warnings.warn( message="Warning: Couldn't determine the LAMMPS to pyiron unit conversion type of quantity " - f"{label}. Returning un-normalized quantity" + f"{label}. Returning un-normalized quantity", + stacklevel=2, )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/lammpsparser/units.py` around lines 240 - 244, The warnings.warn call in the unit-conversion fallback should specify stacklevel=2 so the warning points to the caller rather than the internal parser line. Update this warning invocation while preserving its message and the subsequent np.array(array) return.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/lammpsparser/output.py`:
- Around line 4-7: Complete the pathlib migration in parse_lammps_output by
replacing the remaining os.path.join calls with pathlib.Path-based path
construction and string conversion, preserving the existing working_directory
and filename values; do not leave unresolved os references after removing the
import.
---
Nitpick comments:
In `@src/lammpsparser/units.py`:
- Around line 240-244: The warnings.warn call in the unit-conversion fallback
should specify stacklevel=2 so the warning points to the caller rather than the
internal parser line. Update this warning invocation while preserving its
message and the subsequent np.array(array) return.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dcede283-5096-4b24-8430-d3babe0aceff
📒 Files selected for processing (19)
src/lammpsparser/_version.pysrc/lammpsparser/compatibility/calculate.pysrc/lammpsparser/compatibility/file.pysrc/lammpsparser/compatibility/structure.pysrc/lammpsparser/output.pysrc/lammpsparser/output_raw.pysrc/lammpsparser/potential.pysrc/lammpsparser/structure.pysrc/lammpsparser/units.pytests/test_compatibility_calculate.pytests/test_compatibility_file.pytests/test_compatibility_integration.pytests/test_compatibility_output_structure.pytests/test_compatibility_structure.pytests/test_output.pytests/test_output_raw.pytests/test_potential.pytests/test_structure.pytests/test_units.py
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #381 +/- ##
=======================================
Coverage 98.68% 98.69%
=======================================
Files 13 13
Lines 1219 1224 +5
=======================================
+ Hits 1203 1208 +5
Misses 16 16 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
2 participants
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.
Summary
Repo health: 20/100
Our analysis found ~720 format/lint tidy opportunities and about 183 refactoring opportunities across the reviewed scope. Security scans flagged one
subprocesscall withshell=Trueincompatibility/file.py(bandit B602); code duplication is low (~0.7% in Python modules per jscpd); cyclomatic complexity P95 slightly exceeds the configured ceiling (radon); about 183 structural refactor suggestions remain (repeated strings, class-local helpers, test-only API surface).This pull request is a sample of those findings — 19 files, ~141 focused line changes — not a full format pass or refactor cleanup.
What you are doing right
What you could improve
subprocessis invoked withshell=Trueincompatibility/file.py:252— consider argv list form if the command string is not strictly required. (follow-up)refactor check --strictsuggestions remain — e.g. extract repeated'Structure not set'string constant instructure.pyand move class-local helpers to@staticmethod. (follow-up)lammpsimport intests/test_compatibility_integration.py. (defer — integration test fixture)deptryflaggedtomlkitused in.ci_support/check.pybut not declared inpyproject.toml. (defer — CI script only)pathlib.Pathoveros.path/open()for file existence and reads. (included in example PR)typing.UniontoX | Yand importCallablefromcollections.abc. (included in example PR)elsebranches after early returns and usedict.fromkeyswhere applicable. (included in example PR)Changes
src/lammpsparser/_version.py— modernizeVERSION_TUPLEtype hint to PEP 604 syntax.src/lammpsparser/compatibility/calculate.py— remove redundantelseafter raise; quotecast()type expression.src/lammpsparser/compatibility/file.py— usepathlib.Path.mkdir()andPath.open()for LAMMPS input file writes.src/lammpsparser/compatibility/structure.py— usedict.fromkeysfor charge dict; quotecast()type; tidy comments.src/lammpsparser/output.py— importCallablefromcollections.abc; usePath.is_file()for dump detection.src/lammpsparser/output_raw.py— pathlib and import modernizations.src/lammpsparser/potential.py— import sorting and minor style cleanups.src/lammpsparser/structure.py— import and style tidy-ups.src/lammpsparser/units.py— import modernizations.tests/test_*.py(10 files) — align test imports and pathlib usage with source changes.Review summary produced with ShipGate — a policy-first quality orchestrator that runs linters, formatters, security scanners, and refactor checks from one catalog. This PR used
check+refactor check --strictto find issues; docs.Summary by CodeRabbit
Refactor
Tests