Tidy minor structural overlaps (#107) - #120
Conversation
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Summary
Testing
WalkthroughChangesPublish formatting
Execution context
Catalogue migration
Possibly related PRs
Suggested labels: Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (19 passed)
📋 Issue PlannerBuilt with CodeRabbit's Coding Plans for faster development and fewer bugs. View plan used: ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Reviewer's GuideRefactors publish-plan section rendering into a single reusable render_section helper that returns lines, updates format_plan to use it, tightens CLI configuration scoping in _run_with_context, simplifies live publication error handling, and documents the staged LADING_CATALOGUE plus adds property/snapshot tests to pin behaviours. Sequence diagram for unified _run_with_context execution flowsequenceDiagram
actor User
participant CLI as cli._run_with_context
participant Config as config
participant Meta as metadata_module
participant Workspace as load_workspace
participant Runner as runner
User->>CLI: _run_with_context(workspace_root, runner, command_runner)
CLI->>Config: current_configuration()
alt configuration not loaded
Config-->>CLI: raise ConfigurationNotLoadedError
CLI->>Config: load_configuration(workspace_root)
Config-->>CLI: configuration
CLI->>Config: use_configuration(configuration)
Config-->>CLI: configuration_scope
else configuration already loaded
Config-->>CLI: configuration
Note over CLI: configuration_scope = nullcontext()
end
CLI->>Meta: use_command_runner(active_runner)
activate Meta
CLI->>Workspace: load_workspace(workspace_root)
Workspace-->>CLI: workspace_model
CLI->>Runner: runner(workspace_root, configuration, workspace_model, active_runner)
Runner-->>CLI: result
Meta-->>CLI: exit
CLI-->>User: result
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}
There was a problem hiding this comment.
Our agent can fix these. Install it.
Gates Passed
Absence of Expected Change Pattern
- lading/lading/commands/publish.py is usually changed with: lading/tests/bdd/steps/test_publish_steps.py
- lading/tests/unit/test_cli.py is usually changed with: lading/lading/commands/bump.py
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
c47f324 to
f63b22c
Compare
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?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}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
f63b22c to
1f4ce2a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
1f4ce2a to
af12c3a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
af12c3a to
8977dad
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
8977dad to
20205cb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
db54877 to
8624f6c
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following now been resolved? tests/unit/publish/test_formatting_helpers.py (1)
🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
ast-grep outline tests/unit/publish/test_formatting_helpers.py --items all --type function
printf '\n--- legacy module ---\n'
ast-grep outline tests/unit/test_publish_formatting.py --items all --type function || true
printf '\n--- canonical test region ---\n'
nl -ba tests/unit/publish/test_formatting_helpers.py | sed -n '1,90p'
printf '\n--- legacy test module ---\n'
nl -ba tests/unit/test_publish_formatting.py | sed -n '1,120p' || trueLength of output: 1869
|
|
@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. (Treat documentation and validation coverage as in scope). ❌ Failed checks (2 warnings)
|
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
14a3443 to
d84eefb
Compare
13a: collapse _format_crates_section into the single section renderer. The bespoke function duplicated _append_section plus an empty message and a fixed formatter. The renderer is now _render_section, a query returning the section lines (which also keeps the argument count within the lint ceiling); _format_plan composes it for every section. The historical public append_section (listed in __all__) is retained as a thin backwards-compatible shim that extends the passed-in list by delegating to _render_section, so existing importers keep working while new code uses render_section. 13b: catch PublishPreflightError alone in the live pipeline, with a comment noting that PublishError subclasses it, so the redundant tuple is not reintroduced by accident. 13c: deduplicate the load-workspace-and-run block in cli._run_with_context. Configuration scope is resolved first (a nullcontext when already loaded), then a single block runs the command. 13d: document that utils.commands.LADING_CATALOGUE is staged but not yet wired into the execution path, in both the module docstring and the developers' guide, with a pointer to the roadmap Phase 5 steps that will wire it. Tests: property test pins the render_section header/item/empty-message invariants, unit tests pin the append_section shim (list mutation, empty no-op, parity with render_section), syrupy snapshots lock the fully rendered publish plan with and without publishable crates, and a behavioural test pins identical downstream behaviour for both _run_with_context branches. Closes #107
Address two review warnings on the publish-plan/CLI work. Users' guide: document the publish-plan summary that `lading publish` prints on completion — the crates-to-publish list, the skipped-crate sections, and the `Crates to publish: none` empty state — which was previously covered only by tests and the developers' guide. CLI-boundary test: add `test_publish_via_app_matches_across_config_branches`, which drives `cli.app` (the public command boundary) for both `_run_with_context` configuration branches. `cli.main` always installs configuration before dispatch, so the disk-loaded branch is reachable publicly only through `cli.app` without an active scope. The test asserts identical downstream behaviour (same workspace, equal configuration) while pinning the provenance difference: the disk branch reloads a fresh config object; the pre-loaded branch reuses the active one. The white-box `test_run_with_context_branches_behave_identically` is retained.
Address review comments on the publish-plan/CLI test and docs. Tests: give the previously bare assertions in the render_section unit tests focused diagnostic messages — the default-formatting, empty-section, empty-message, the four property invariants, and both snapshot assertions — plus the `minimal_config.exists()` setup check in the CLI config-branch test. Each message states the contract being verified so a failure explains itself. Docs: turn the code-formatted `docs/roadmap.md` reference in the LADING_CATALOGUE / Phase 5 text into a relative Markdown link, `[Phase 5 runner-migration steps](./roadmap.md)`, matching the guide's existing relative-link style.
Address review comments on the publish-plan/CLI work. Tests: give the config-branch tests (`test_run_with_context_branches_ behave_identically`, `test_publish_via_app_matches_across_config_branches`) and the remaining section-helper tests diagnostic assertion messages, so a failure names the invariant it checks (result, call count, workspace root, configuration object, or workspace graph). No test logic changes. Users' guide: move the `Crates to publish: none` empty-state sentence so it introduces the empty-case example rather than the populated one. Developers' guide: document the `publish_plan` section helpers (`render_section`, `append_section`, `format_plan`) under the extracted publish modules, and add a "CLI context loading" subsection describing `_run_with_context` and its single-branch configuration resolution.
Post-rebase reconciliation with main's #176 (plaintext prose fences): convert the two publish-plan example fences in the users' guide from `text` to `plaintext` to match the repository convention, and drop a duplicate blank line that the entity merge left before the new "CLI context loading" subsection in the developers' guide (MD012).
d84eefb to
bf456eb
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
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)
tests/unit/test_publish_formatting.py (1)
17-42: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the duplicate skipped-section test.
Retain the canonical coverage in
tests/unit/publish/test_formatting_helpers.pyand delete this identical test.
As per coding guidelines, “Extract reusable logic, avoid repetition”.Proposed fix
-def test_format_plan_formats_skipped_sections( - tmp_path: Path, - make_crate: cabc.Callable[[Path, str, _CrateSpec | None], WorkspaceCrate], -) -> None: - """``format_plan`` renders skipped crates using their names only.""" - ...🤖 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 `@tests/unit/test_publish_formatting.py` around lines 17 - 42, Delete the duplicate test_format_plan_formats_skipped_sections test from tests/unit/test_publish_formatting.py, while retaining the canonical coverage in test_formatting_helpers.py. Do not modify the canonical test or add replacement coverage.Source: Coding guidelines
🤖 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 `@lading/utils/commands.py`:
- Around line 12-16: Update the migration documentation in the subprocess-runner
context to reference `_invoke_via_subprocess()` in `publish_execution.py` as the
Phase 5.2 target, replacing the incorrect `lading/runtime/subprocess_runner.py`
reference while preserving the existing note that the catalogue is not yet wired
into execution.
---
Outside diff comments:
In `@tests/unit/test_publish_formatting.py`:
- Around line 17-42: Delete the duplicate
test_format_plan_formats_skipped_sections test from
tests/unit/test_publish_formatting.py, while retaining the canonical coverage in
test_formatting_helpers.py. Do not modify the canonical test or add replacement
coverage.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 59dfcff0-4d91-4008-8404-05cb64b678d2
📒 Files selected for processing (10)
docs/developers-guide.mddocs/users-guide.mdlading/cli.pylading/commands/publish.pylading/commands/publish_plan.pylading/utils/commands.pytests/unit/publish/__snapshots__/test_formatting_helpers.ambrtests/unit/publish/test_formatting_helpers.pytests/unit/test_cli.pytests/unit/test_publish_formatting.py
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/cmd-mox(auto-detected)leynos/cuprum(auto-detected)leynos/shared-actions(auto-detected)
|
@coderabbitai Have the following now been resolved? tests/unit/publish/test_formatting_helpers.py (1)
🛠️ Proposed fix- assert lines == ["Header:", "- ALPHA", "- BETA"]
+ assert lines == ["Header:", "- ALPHA", "- BETA"], (
+ "formatter should be applied to each item under the header"
+ )As per path instructions, "Use 🤖 Detailed instructionsVerify each finding against current code. Fix only still-valid issues, skip the In Source: Path instructions |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- test-function outline ---'
ast-grep outline tests/unit/publish/test_formatting_helpers.py --items all --type function
printf '%s\n' '--- relevant assertions ---'
nl -ba tests/unit/publish/test_formatting_helpers.py | sed -n '15,105p'
printf '%s\n' '--- repository instruction files, if present ---'
fd -a -t f -i 'AGENTS.md' -i 'CONTRIBUTING.md' -i '*instruction*' . \
-E .git -E .venv -E node_modules | head -50Length of output: 1707 🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant assertions ---'
sed -n '15,105p' tests/unit/publish/test_formatting_helpers.py
printf '%s\n' '--- instruction-file candidates ---'
fd -a -t f . -E .git -E .venv -E node_modules \
| grep -Ei '(^|/)(AGENTS\.md|CONTRIBUTING\.md|.*instructions?.*|.*guidelines?.*)$' \
| head -50 || true
printf '%s\n' '--- assertion-message guidance ---'
rg -n -i -C 2 'assert.*message|message.*assert|bare assert' \
--glob 'AGENTS.md' \
--glob 'CONTRIBUTING.md' \
--iglob '*instruction*' \
--iglob '*guideline*' \
. || trueLength of output: 3903
The test logic and expected values remain unchanged. |
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
`test_format_plan_formats_skipped_sections` in test_publish_formatting.py was a behavioural duplicate of the canonical test in tests/unit/publish/test_formatting_helpers.py (identical assertions on `format_plan` skipped-section rendering; only the crate-construction plumbing differed). Remove it and the imports it alone used (`collections.abc`, `publish_plan`, `_CrateSpec`, `WorkspaceCrate`); the file keeps its unique `test_format_preparation_summary` coverage.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
The catalogue-wiring migration target was misrouted to the low-level `subprocess_runner.py` (and cited a roadmap step that does not exist). The catalogue's `scoped(allowlist=…)` enforcement is wired in by roadmap Phase 5.2 "Publish Execution Migration", which rewires `publish_execution.py`'s command execution (the roadmap's `_invoke_via_subprocess()` step; `_invoke` today) onto cuprum. Point the `utils/commands.py` module docstring and the developers' guide note at that step, keeping the "staged, not yet wired" caveat.
There was a problem hiding this comment.
Our agent can fix these. Install it.
Gates Passed
Absence of Expected Change Pattern
- lading/lading/commands/publish.py is usually changed with: lading/tests/bdd/steps/test_publish_steps.py
- lading/tests/unit/test_cli.py is usually changed with: lading/lading/commands/bump.py
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
|
@coderabbitai review |
✅ Action performedReview finished.
|
3 participants
Summary
Closes #107
_format_crates_sectionfolded into the single section renderer. The renderer is now_render_section, a query returning the section lines (also keeps the signature within the four-argument lint ceiling), with an optionalempty_message;_format_plancomposes it for every section.PublishPreflightErroralone, with a comment noting thatPublishErrorsubclasses it so the redundant tuple is not reintroduced by accident.cli._run_with_contextresolves the configuration scope first (anullcontextwhen already loaded) and runs a single load-workspace-and-run block.utils.commands.LADING_CATALOGUEis documented as staged-but-not-wired in both the module docstring and the developers' guide, pointing at the roadmap Phase 5 wiring steps.Testing
render_sectionheader/item/empty-message invariants._run_with_contextbranches.make check-fmt,make lint,make typecheck, andmake test(560 passed) all green.coderabbit review --agent: 0 findings.🤖 Generated with Claude Code
Summary by Sourcery
Refine publish plan rendering and CLI execution context handling while clarifying command catalogue documentation and strengthening tests around these behaviours.
Enhancements:
Documentation:
Tests:
References