Drop plumbum round-trip in normalise_workspace_root (#101) - #110
Conversation
Replace the pointless plumbum local.path round-trip with a pure pathlib implementation. The plumbum conversion produced a string that was immediately re-wrapped in Path, adding nothing while keeping a production dependency the cuprum migration is retiring. Move plumbum to the dev dependency group: the end-to-end test helpers still shell out through it, but no production module imports it. Add unit and Hypothesis property tests pinning behaviour parity with the previous implementation (absolute results, tilde expansion, separator normalisation, and the None-to-cwd default), and update the design document and roadmap to record the migration. Closes #101
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughReplace production plumbum path handling with pure pathlib in normalise_workspace_root, add Hypothesis and unit tests for its behaviour, move plumbum to dev dependencies, and update design, developers guide, and roadmap documentation. ChangesPath normalisation modernisation
Sequence DiagramsequenceDiagram
participant Caller
participant normalise_workspace_root
participant pathlib_Path
Caller->>normalise_workspace_root: pass value (None | str | Path)
normalise_workspace_root->>pathlib_Path: Path(value).expanduser().resolve(strict=False)
pathlib_Path-->>normalise_workspace_root: Path result
normalise_workspace_root-->>Caller: return resolved Path
🚥 Pre-merge checks | ✅ 18 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (18 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideMigrates workspace root normalisation to a pure pathlib implementation, removes plumbum from runtime dependencies, and adds tests and docs updates to lock in the new behaviour. Flow diagram for the pathlib-based normalise_workspace_rootflowchart TD
A["call normalise_workspace_root(value)"] --> B{"value is None"}
B -- Yes --> C["Path.cwd().resolve()"]
C --> D["return Path.cwd().resolve()"]
B -- No --> E["Path(value)"]
E --> F[".expanduser()"]
F --> G[".resolve(strict=False)"]
G --> H["return expanded Path"]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Build the property-test path inputs from segment lists joined via `pathlib.Path` (and `os.sep` for separator doubling) instead of hard-coded `/`, so the suite behaves identically on Windows. Also constrain the `bump_readme` fenced-code-block property test so the generated body cannot contain its own fence delimiters; a body that opened a code block would be closed by `fence_header`, leaving the link outside any fence and correctly rewritten — a flaw in the test, not the production code. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Strengthen `test_accepts_path_instances` with a concrete oracle (`Path.home().resolve() / "ws"`) so it no longer passes against a constant-returning implementation; the Path-equals-str check is retained as a secondary assertion. Document `normalise_workspace_root` in the developers' guide: its `Path`/`str`/`None` contract, the pathlib-only implementation, and the demotion of `plumbum` to a dev-only dependency. Correct the now-stale `load_cargo_metadata` note that claimed it shelled out via `plumbum`; it uses the active `CommandRunner`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
cat tests/unit/utils/test_path.pyLength of output: 2591
The strategies and tests are now platform-agnostic. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
Replace the pathlib mirror oracle in the path property tests, which
restated the implementation almost verbatim, with genuinely independent
metamorphic invariants: results are absolute, retain no unresolved
`.`/`..` segments, anchor relative inputs at the cwd, are fixed points
of further normalisation, and expand `~` to exactly the home directory.
Correct US spellings ("normalizes"/"normalization") to en-GB
("normalises"/"normalisation") throughout the developers' guide, and
drop the stale claim in the design doc that workspace discovery shells
out via `plumbum`; it uses the active `CommandRunner`.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@coderabbitai Have the following now been resolved? docs/lading-design.md (1)
now invokes design text is inaccurate. 📝 Patch the wording-- Workspace discovery is anchored in `lading.workspace.metadata`. The module
- uses `plumbum` to construct `cargo metadata --format-version 1`, normalising
- the workspace root via `lading.utils.normalise_workspace_root` before
- invoking the command.
+- Workspace discovery is anchored in `lading.workspace.metadata`. The module
+ normalises the workspace root via `lading.utils.normalise_workspace_root`,
+ then invokes `cargo metadata --format-version 1` through the active
+ command-runner boundary.As per coding guidelines, keep design documents synchronized with the implemented system. 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Coding guidelines |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
sed -n '220,240p' docs/lading-design.mdLength of output: 1339
The stale plumbum claim is gone and the text correctly reflects the |
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. Do not treat warnings as optional or aspirational. Where a change is out of scope for this PR, propose a GitHub issue unless one exists already. ❌ Failed checks (2 warnings)
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
echo "=== Checking 'normalizes' vs 'normalises' in docs/developers-guide.md ==="
rg -n "normaliz" docs/developers-guide.md
echo ""
echo "=== Lines 190-200, 383-393, 494-504 ==="
sed -n '190,200p' docs/developers-guide.md
echo "---"
sed -n '383,393p' docs/developers-guide.md
echo "---"
sed -n '494,504p' docs/developers-guide.md
echo ""
echo "=== Full tests/unit/utils/test_path.py ==="
cat tests/unit/utils/test_path.pyLength of output: 5146
Developer Documentation ✅ Testing (Property / Proof) ✅
None of these re-implement the production logic, so the oracle is no longer tautological. |
1 participant
Summary
Closes #101
local.pathround-trip innormalise_workspace_rootwith a purepathlibimplementation (Path(value).expanduser().resolve(strict=False)).plumbumfrom production dependencies to the dev group: only the end-to-end test helpers still use it; no production module imports it any longer.uv.lockaccordingly.docs/lading-design.mdand tick the correspondingdocs/roadmap.mdPhase 5 item.Testing
tests/unit/utils/test_path.pywith unit tests plus Hypothesis property tests pinning parity with the previous behaviour: results are absolute,~expands, redundant separators normalise,Noneselects the resolved cwd.make check-fmt,make lint,make typecheck, andmake test(561 passed) all green.🤖 Generated with Claude Code
Summary by Sourcery
Migrate workspace path normalisation to a pure pathlib implementation and adjust dependencies, docs, and tests accordingly.
Enhancements:
Build:
Documentation:
Tests: