-
Notifications
You must be signed in to change notification settings - Fork 5
Follow Python standard packaging guidelines #389
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
WalkthroughThe repository is moved to a src-layout: CI workflows and build tooling references updated to use Changes
Sequence Diagram(s)No sequence diagram included — changes are limited to path/config updates with no new runtime control flow. Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
==========================================
+ Coverage 85.17% 85.41% +0.23%
==========================================
Files 6 7 +1
Lines 796 809 +13
==========================================
+ Hits 678 691 +13
Misses 118 118 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
44-47: Update[tool.hatch.build]include for consistency with src-layout and best practices.Global build configuration in the tool.hatch.build table is not recommended when target-specific config is present, as keys may then be overridden by target config. The global
include = ["pylammpsmpi"](line 46) refers to the root directory structure, while the build targets specifypackages = ["src/pylammpsmpi"](lines 93, 98). When packages is defined for a target, it relies on sources and takes precedence.While hatch will correctly build using the target-specific packages, align the global include with the src-layout for consistency and clarity:
[tool.hatch.build] include = [ - "pylammpsmpi" + "src/pylammpsmpi" ]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/deploy.yml(1 hunks).github/workflows/pipeline.yml(2 hunks)pyproject.toml(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: codecov/patch
- GitHub Check: unittest_openmpi_mac
- GitHub Check: unittest_mpich_mac
🔇 Additional comments (2)
.github/workflows/deploy.yml (1)
35-35: Version file path correctly updated for src-layout.The git update-index command now correctly references the version file at its new location
src/pylammpsmpi/_version.py, consistent with pyproject.toml configuration..github/workflows/pipeline.yml (1)
16-16: Black formatter paths correctly updated for src-layout.Both the check job and auto-fix job now reference the source directory at its new location under
src/. The paths are consistent and will correctly format the migrated package structure.Also applies to: 32-32
|
|
||
| [tool.coverage.run] | ||
| omit = ["pylammpsmpi/_version.py", "tests/*"] | ||
| source = ["pylammpsmpi"] |
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.
Coverage source path not updated to src-layout.
Line 106 still references the old package location ["pylammpsmpi"] instead of ["src/pylammpsmpi"]. This will cause coverage collection to fail after migration to the src-layout structure, as it won't find source files in their new location.
Apply this fix:
[tool.coverage.run]
-source = ["pylammpsmpi"]
+source = ["src/pylammpsmpi"]
command_line = "-m unittest discover tests"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| source = ["pylammpsmpi"] | |
| [tool.coverage.run] | |
| source = ["src/pylammpsmpi"] | |
| command_line = "-m unittest discover tests" |
🤖 Prompt for AI Agents
In pyproject.toml around line 106, the coverage source path still points to the
old package root ["pylammpsmpi"] but the repository uses src-layout; update the
path to ["src/pylammpsmpi"] so coverage tools can find sources under the src/
directory and regenerate/verify coverage by running the coverage collection/test
command after the change.
Summary by CodeRabbit