Plan publishable crates before publish - #16
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds publication planning to the publish command: introduces a PublishPlan model and plan_publication() to classify crates as publishable, manifest-skipped, configuration-excluded, and report missing exclusions; integrates plan formatting into run, updates docs, and adds/rewires BDD and unit tests and fixtures. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as "lading publish CLI"
participant Workspace as "WorkspaceGraph / cargo metadata"
participant Config as "LadingConfig (lading.toml)"
participant Formatter as "Plan Formatter"
User->>CLI: invoke publish
CLI->>Workspace: load workspace graph / cargo metadata
CLI->>Config: load configuration (publish.exclude)
CLI->>CLI: plan_publication(workspace, configuration)
Note right of CLI #f7f7d9: classify crates into\n- publishable\n- manifest-skipped\n- config-excluded\n- collect missing exclusions
CLI->>Formatter: _format_plan(plan, strip_patches)
Formatter-->>CLI: formatted plan text
CLI-->>User: print plan summary (stdout)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Files/areas to pay extra attention to:
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (.rules/python-00.md)
Files:
{**/unittests/test_*.py,tests/**/*.py}📄 CodeRabbit inference engine (.rules/python-00.md)
Files:
tests/**/*.py📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
Files:
🧬 Code graph analysis (1)tests/bdd/steps/fixtures.py (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)
🔇 Additional comments (5)
Comment |
![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.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +73 to +100 def _format_plan(
plan: PublishPlan, *, strip_patches: config_module.StripPatchesSetting
) -> str:
"""Render ``plan`` to a human-readable summary for CLI output."""
lines = [
f"Publish plan for {plan.workspace_root}",
f"Strip patch strategy: {strip_patches}",
]
if plan.publishable:
lines.append(f"Crates to publish ({len(plan.publishable)}):")
lines.extend(f"- {crate.name} @ {crate.version}" for crate in plan.publishable)
else:
lines.append("Crates to publish: none")
if plan.skipped_manifest:
lines.append("Skipped (publish = false):")
lines.extend(f"- {crate.name}" for crate in plan.skipped_manifest)
if plan.skipped_configuration:
lines.append("Skipped via publish.exclude:")
lines.extend(f"- {crate.name}" for crate in plan.skipped_configuration)
if plan.missing_configuration_exclusions:
lines.append("Configured exclusions not found in workspace:")
lines.extend(f"- {name}" for name in plan.missing_configuration_exclusions)
return "\n".join(lines)❌ New issue: Complex Method |
This comment was marked as resolved.
This comment was marked as resolved.
|
@sourcery-ai review |
Reviewer's GuideThis PR implements a full publish planning workflow by introducing a PublishPlan model and planning function, integrating it into the CLI command in place of a placeholder implementation, updating documentation and configuration scripts, and adding comprehensive unit and BDD tests to verify the new behavior. Sequence diagram for the new publish planning workflow in CLIsequenceDiagram
participant User as actor User
participant CLI as "lading publish CLI"
participant Config as "LadingConfig"
participant Workspace as "WorkspaceGraph"
participant Planner as "plan_publication()"
participant Formatter as "_format_plan()"
User->>CLI: Run publish command
CLI->>Config: Ensure configuration loaded
CLI->>Workspace: Ensure workspace loaded
CLI->>Planner: Plan publication (workspace, config)
Planner->>Planner: Filter crates by manifest and config
Planner-->>CLI: Return PublishPlan
CLI->>Formatter: Format PublishPlan for output
Formatter-->>CLI: Return formatted plan
CLI-->>User: Display publication plan
Class diagram for the new PublishPlan and planning workflowclassDiagram
class PublishPlan {
+Path workspace_root
+tuple[WorkspaceCrate] publishable
+tuple[WorkspaceCrate] skipped_manifest
+tuple[WorkspaceCrate] skipped_configuration
+tuple[str] missing_configuration_exclusions
+publishable_names(): tuple[str]
}
class WorkspaceCrate {
+str name
+str version
+bool publish
}
class WorkspaceGraph {
+Path workspace_root
+list[WorkspaceCrate] crates
}
class LadingConfig {
+PublishConfig publish
}
class PublishConfig {
+list[str] exclude
+str strip_patches
}
PublishPlan "1" -- "*" WorkspaceCrate : publishable
PublishPlan "1" -- "*" WorkspaceCrate : skipped_manifest
PublishPlan "1" -- "*" WorkspaceCrate : skipped_configuration
WorkspaceGraph "1" -- "*" WorkspaceCrate : crates
LadingConfig "1" -- "1" PublishConfig : publish
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![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.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Have the following now been resolved?
|
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/bdd/steps/test_cli_steps.py Comment on file version: str,
manifest_path: Path,
dependencies: list[dict[str, str]] | None = None,
*,❌ New issue: Low Cohesion |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: tests/bdd/steps/test_cli_steps.py Comment on lines +220 to +221 *,
publish: bool | tuple[str, ...] | None = None,❌ New issue: Excess Number of Function Arguments |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 2434 @leynos: Let me check the current state of the code: Status Update:
To resolve the sorting issue, you could modify def _format_section(
items: tuple[T, ...],
*,
header: str,
item_formatter: typ.Callable[[T], str],
empty_lines: tuple[str, ...] = (),
sort_key: typ.Callable[[T], typ.Any] | None = None,
) -> list[str]:
"""Return ``header`` and formatted ``items`` when any are present."""
if not items:
return list(empty_lines)
sorted_items = sorted(items, key=sort_key) if sort_key else items
formatted_items = [item_formatter(item) for item in sorted_items]
return [header, *formatted_items]Then update the calls in |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file version: str,
@given(parsers.parse('publish.exclude contains "{crate_name}"'))
def given_publish_exclude_contains(
workspace_directory: Path,❌ New issue: Code Duplication |
This comment was marked as resolved.
This comment was marked as resolved.
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
tests/bdd/steps/fixtures.py (1)
370-377: Nice deduplication via _add_exclude_to_config.This removes the previous code duplication flagged by CodeScene; cleaner and single source of truth.
Also applies to: 379-385
🧹 Nitpick comments (9)
lading/commands/publish.py (2)
83-97: Reduce _format_section argument count to satisfy CodeScene and simplify call sites._currently 5 params; CodeScene flags “Excess Number of Function Arguments” (max=4). Remove sort_key and pre‑sort at call sites. Output stays identical.
Apply:
-def _format_section( - items: tuple[T, ...], - *, - header: str, - item_formatter: typ.Callable[[T], str], - empty_lines: tuple[str, ...] = (), - sort_key: typ.Callable[[T], typ.Any] | None = None, -) -> list[str]: +def _format_section( + items: tuple[T, ...], + *, + header: str, + item_formatter: typ.Callable[[T], str], + empty_lines: tuple[str, ...] = (), +) -> list[str]: @@ - ordered_items = sorted(items, key=sort_key) if sort_key else items - formatted_items = [item_formatter(item) for item in ordered_items] + formatted_items = [item_formatter(item) for item in items] return [header, *formatted_items]And update call sites:
lines.extend( _format_section( - plan.publishable, + plan.publishable, header=f"Crates to publish ({len(plan.publishable)}):", item_formatter=lambda crate: f"- {crate.name} @ {crate.version}", empty_lines=("Crates to publish: none",), - sort_key=lambda crate: crate.name, ) ) @@ lines.extend( _format_section( - plan.skipped_manifest, + plan.skipped_manifest, header="Skipped (publish = false):", item_formatter=lambda crate: f"- {crate.name}", - sort_key=lambda crate: crate.name, ) ) @@ lines.extend( _format_section( - plan.skipped_configuration, + plan.skipped_configuration, header="Skipped via publish.exclude:", item_formatter=lambda crate: f"- {crate.name}", - sort_key=lambda crate: crate.name, ) ) @@ lines.extend( _format_section( - plan.missing_configuration_exclusions, + tuple(sorted(plan.missing_configuration_exclusions)), header="Configured exclusions not found in workspace:", item_formatter=lambda name: f"- {name}", - sort_key=lambda name: name, ) )This addresses the CodeScene biomarker without changing behaviour. As per coding guidelines.
Also applies to: 109-141
50-51: Avoid redundant sorting; sort once at the end.You sort workspace.crates (Line 50) and then sort the result lists again (Lines 66–73). One sort is enough.
Apply:
- workspace_crates = tuple(sorted(workspace.crates, key=lambda crate: crate.name)) + workspace_crates = workspace.cratesKeep the ordered_* sorts, which produce the stable output. Simpler and equivalent. As per coding guidelines.
Also applies to: 66-73
tests/bdd/steps/test_publish_steps.py (1)
31-37: Reuse the helper to trim lines; remove duplication.Use _publish_plan_lines for consistency with other steps.
Apply:
- workspace = cli_run["workspace"] - lines = [line.strip() for line in cli_run["stdout"].splitlines() if line.strip()] + workspace = cli_run["workspace"] + lines = _publish_plan_lines(cli_run)tests/bdd/steps/test_bump_steps.py (1)
88-91: Make expectation robust to the “- ” prefix.If the feature inputs are bare paths, prefix them here to match CLI output.
Apply:
- expected_lines = [first, second] + expected_lines = [f"- {first}", f"- {second}"]tests/bdd/steps/test_common_steps.py (2)
24-49: Prefer a TypedDict for CLI run results; avoid Any.Define a CLIRun TypedDict and use it for _run_cli and step params. Clearer types and better IDE help.
Apply:
-from pytest_bdd import parsers, scenarios, then +from pytest_bdd import parsers, scenarios, then @@ -if typ.TYPE_CHECKING: - from pathlib import Path +from typing import TypedDict +if typ.TYPE_CHECKING: + from pathlib import Path + +class CLIRun(TypedDict): + returncode: int + stdout: str + stderr: str + workspace: "Path" @@ -def _run_cli( +def _run_cli( repo_root: Path, workspace_directory: Path, *command_args: str, -) -> dict[str, typ.Any]: +) -> CLIRun: @@ - return { + return { "returncode": completed.returncode, "stdout": completed.stdout, "stderr": completed.stderr, "workspace": workspace_directory.resolve(), }Then update step signatures in this module to use CLIRun instead of dict[str, typ.Any]. As per coding guidelines.
189-191: Avoid duplicate step registration.These imports duplicate plugin loading already done in tests/conftest.py. Remove to reduce noise and potential double-import side effects.
Apply:
-# Import subcommand-specific steps so their definitions register with pytest-bdd. -from . import test_bump_steps as _bump_steps # noqa: E402,F401 # isort: skip -from . import test_publish_steps as _publish_steps # noqa: E402,F401 # isort: skip +# Step modules are registered via pytest_plugins in tests/conftest.py.tests/bdd/steps/fixtures.py (3)
33-46: Avoid KeyError when [workspace.package] is missing.Accessing workspace_document["workspace"]["package"] can fail if the table isn’t present in scenarios that don’t pre-create it. Init tables defensively.
- workspace_document = parse_toml(workspace_manifest.read_text(encoding="utf-8")) - workspace_document["workspace"]["package"]["version"] = version + workspace_document = parse_toml(workspace_manifest.read_text(encoding="utf-8")) + # Be tolerant if [workspace] or [workspace.package] is missing + ws = workspace_document.get("workspace") or table() + workspace_document["workspace"] = ws + pkg = ws.get("package") or table() + ws["package"] = pkg + pkg["version"] = version
209-226: Tighten types for cargo metadata; drop dict[str, Any].Use TypedDicts for metadata payloads and refine _build_package_metadata types. This improves static coverage and avoids Any. As per coding guidelines.
Add near imports:
- from tomlkit import array, table + from tomlkit import array, tableAdd TypedDicts (place after imports):
+class DependencyEntry(typ.TypedDict, total=False): + name: str + package: str + # Only when present in cargo metadata + kind: typ.Literal["dev", "build"] + +class PackageMetadata(typ.TypedDict): + name: str + version: str + id: str + manifest_path: str + dependencies: list[DependencyEntry] + publish: typ.NotRequired[bool | tuple[str, ...] | None]Update signature and return type:
-def _build_package_metadata( - name: str, - manifest_path: Path, - version: str = "0.1.0", - dependencies: list[dict[str, str]] | None = None, - *, - publish: bool | tuple[str, ...] | None = None, -) -> dict[str, typ.Any]: +def _build_package_metadata( + name: str, + manifest_path: Path, + version: str = "0.1.0", + dependencies: list[DependencyEntry] | None = None, + *, + publish: bool | tuple[str, ...] | None = None, +) -> PackageMetadata:Optional (if you want to also reduce “argument count” metrics): introduce a small options object and make dependencies/publish fields of it; happy to draft that if desired.
349-368: Make _add_exclude_to_config resilient when config is absent.Currently assumes the config file exists; calling after “workspace without configuration” would raise. Create a new TOML document when missing.
-from tomlkit import parse as parse_toml +from tomlkit import parse as parse_toml +from tomlkit import document as new_document @@ def _add_exclude_to_config( @@ - document = parse_toml(config_path.read_text(encoding="utf-8")) + if config_path.exists(): + document = parse_toml(config_path.read_text(encoding="utf-8")) + else: + document = new_document() @@ - config_path.write_text(document.as_string(), encoding="utf-8") + config_path.write_text(document.as_string(), encoding="utf-8")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (10)
lading/commands/publish.py(1 hunks)tests/bdd/features/cli.feature(1 hunks)tests/bdd/steps/__init__.py(1 hunks)tests/bdd/steps/fixtures.py(1 hunks)tests/bdd/steps/test_bump_steps.py(1 hunks)tests/bdd/steps/test_cli_steps.py(0 hunks)tests/bdd/steps/test_common_steps.py(1 hunks)tests/bdd/steps/test_publish_steps.py(1 hunks)tests/conftest.py(1 hunks)tests/unit/test_publish_command.py(1 hunks)
💤 Files with no reviewable changes (1)
- tests/bdd/steps/test_cli_steps.py
✅ Files skipped from review due to trivial changes (1)
- tests/bdd/steps/init.py
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/test_publish_command.py
- tests/bdd/features/cli.feature
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*.py: Python files must use snake_case filenames (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants must use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single leading underscore
Use typing everywhere and maintain full static type coverage
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only usage
Avoid Any; prefer precise types (TypeVar, Protocol, Literal, Union); use typing.cast only when necessary with justification; use object for unknown opaque values
Be explicit with return types (e.g., -> None, -> str) for all public functions and methods
Favor immutability: prefer tuples to lists and MappingProxyType for read-only mappings; document third-party frozendict if used
Use# pyright: ignoresparingly and include an explanation comment when used
Avoid side effects at import time (no global state mutation or actions on import)
Use docstrings (NumPy format) for public functions, classes, and modules
Explain tricky code with inline comments
**/*.py: Prefer context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual try/finally blocks
Use contextlib.contextmanager (@contextmanager) to implement simple, linear setup/teardown context managers
Implement a class with enter and exit for context managers that require internal state or more complex lifecycle handling
For file I/O, prefer with open(...) as ... over open()/try/finally/close patterns
Choose @contextmanager when control flow is linear and no persistent state is needed
Choose a class-based context manager when there is internal state, lifecycle methods, re-entry, or advanced context needs
**/*.py: Exception classes must end with the suffix 'Error' (N818)
Prefer specific built-ins (e.g., TypeError, ValueError) or domain exceptions over raising Exception directly (TRY003/TRY004)
Preserve causal chains...
Files:
tests/conftest.pytests/bdd/steps/test_bump_steps.pytests/bdd/steps/fixtures.pytests/bdd/steps/test_publish_steps.pylading/commands/publish.pytests/bdd/steps/test_common_steps.py
{**/unittests/test_*.py,tests/**/*.py}
📄 CodeRabbit inference engine (.rules/python-00.md)
{**/unittests/test_*.py,tests/**/*.py}: Use pytest idioms: prefer fixtures over setup/teardown, parametrize broadly, avoid unnecessary mocks
Group related tests using classes with method names prefixed by test_
Write tests from a user's perspective: test public behaviour, not internals
Files:
tests/conftest.pytests/bdd/steps/test_bump_steps.pytests/bdd/steps/fixtures.pytests/bdd/steps/test_publish_steps.pytests/bdd/steps/test_common_steps.py
tests/**/*.py
📄 CodeRabbit inference engine (.rules/python-exception-design-raising-handling-and-logging.md)
In tests, assert specific exception types (and optional message via regex) rather than using broad Exception (B017)
Files:
tests/conftest.pytests/bdd/steps/test_bump_steps.pytests/bdd/steps/fixtures.pytests/bdd/steps/test_publish_steps.pytests/bdd/steps/test_common_steps.py
🧬 Code graph analysis (5)
tests/bdd/steps/test_bump_steps.py (2)
tests/bdd/steps/test_common_steps.py (1)
_run_cli(24-49)tests/conftest.py (1)
repo_root(21-23)
tests/bdd/steps/fixtures.py (1)
tests/helpers/workspace_helpers.py (1)
install_cargo_stub(15-38)
tests/bdd/steps/test_publish_steps.py (2)
tests/bdd/steps/test_common_steps.py (1)
_run_cli(24-49)tests/conftest.py (1)
repo_root(21-23)
lading/commands/publish.py (3)
lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/config.py (4)
LadingConfig(82-103)current_configuration(155-161)ConfigurationNotLoadedError(27-28)load_configuration(139-142)lading/workspace/models.py (4)
WorkspaceCrate(31-41)WorkspaceGraph(44-53)WorkspaceModelError(18-19)load_workspace(56-63)
tests/bdd/steps/test_common_steps.py (1)
tests/conftest.py (1)
repo_root(21-23)
🔇 Additional comments (4)
lading/commands/publish.py (1)
30-34: LGTM: publishable_names is clear and efficient.Straightforward, typed, and matches the dataclass contract.
tests/conftest.py (1)
12-17: LGTM: plugin registration for BDD steps and fixtures.The plugin list wires step modules and fixtures cleanly for discovery.
tests/bdd/steps/fixtures.py (2)
228-287: LGTM: internal-dependency stub covers dev/build/runtime kinds.Good coverage for multiple sections; entries match cargo metadata shape.
23-31: Configuration placement of strip_patches is correct—no changes needed.Verification confirms
strip_patchesbelongs under[publish]and is correctly placed in the fixture. All usages across the codebase—config loading, tests, and documentation—consistently treat it as a[publish]setting. The fixture code at lines 23–31 is accurate.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file workspace: WorkspaceGraph | None = None,
) -> str:
"""Return a placeholder message for the publish command."""
"""Plan crate publication for ``workspace_root``."""❌ New issue: Bumpy Road Ahead |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
lading/commands/publish.py (1)
50-72: Remove redundant sorting at line 50.Line 50 sorts
workspace.cratesby name, but lines 66–72 re-sort the filtered lists before converting them to tuples. Since the intermediate lists are sorted again later, the initial sort at line 50 is unnecessary.Apply this diff to remove the redundant sort:
- workspace_crates = tuple(sorted(workspace.crates, key=lambda crate: crate.name)) + workspace_crates = workspace.crates crate_names = {crate.name for crate in workspace_crates}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
lading/commands/publish.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
**/*.py: Python files must use snake_case filenames (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants must use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single leading underscore
Use typing everywhere and maintain full static type coverage
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only usage
Avoid Any; prefer precise types (TypeVar, Protocol, Literal, Union); use typing.cast only when necessary with justification; use object for unknown opaque values
Be explicit with return types (e.g., -> None, -> str) for all public functions and methods
Favor immutability: prefer tuples to lists and MappingProxyType for read-only mappings; document third-party frozendict if used
Use# pyright: ignoresparingly and include an explanation comment when used
Avoid side effects at import time (no global state mutation or actions on import)
Use docstrings (NumPy format) for public functions, classes, and modules
Explain tricky code with inline comments
**/*.py: Prefer context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual try/finally blocks
Use contextlib.contextmanager (@contextmanager) to implement simple, linear setup/teardown context managers
Implement a class with enter and exit for context managers that require internal state or more complex lifecycle handling
For file I/O, prefer with open(...) as ... over open()/try/finally/close patterns
Choose @contextmanager when control flow is linear and no persistent state is needed
Choose a class-based context manager when there is internal state, lifecycle methods, re-entry, or advanced context needs
**/*.py: Exception classes must end with the suffix 'Error' (N818)
Prefer specific built-ins (e.g., TypeError, ValueError) or domain exceptions over raising Exception directly (TRY003/TRY004)
Preserve causal chains...
Files:
lading/commands/publish.py
🧬 Code graph analysis (1)
lading/commands/publish.py (3)
lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/config.py (4)
LadingConfig(82-103)current_configuration(155-161)ConfigurationNotLoadedError(27-28)load_configuration(139-142)lading/workspace/models.py (4)
WorkspaceCrate(31-41)WorkspaceGraph(44-53)WorkspaceModelError(18-19)load_workspace(56-63)
⏰ 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)
- GitHub Check: Sourcery review
|
@coderabbitai Have the following now been resolved?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 6170 @leynos: Yes, both improvements have been fully resolved! ✅ Here's what was implemented: 1. Alphabetical Sorting ✅ RESOLVEDThe
This ensures stable, reproducible CLI output regardless of workspace graph ordering. 2. Boilerplate Reduction ✅ RESOLVEDThe code now uses a single generic
This elegantly replaces the need for separate Minor note: Items are also sorted in |
Reintroduce the README fixture moved from main while keeping the BDD step reorganisation and tighten the helper metadata typing to avoid Any usage. Also fix the publication docstring formatting so lint passes after the rebase.
b5b9d4a to
e7f3966
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on lines +74 to +76 crates_root = workspace_directory / "crates"
if not crates_root.exists():
message = f"Crates directory not found: {crates_root}"❌ New issue: Complex Method |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Comment on file "id": f"{name}-id",
"manifest_path": str(manifest_path),
"dependencies": [] if dependencies is None else dependencies,
"publish": publish,❌ New issue: Excess Number of Function Arguments |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
@sourcery-ai review |
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `tests/bdd/steps/test_publish_steps.py:28-36` </location>
<code_context>
+ return _run_cli(repo_root, workspace_directory, "publish")
+
+
+@then(parsers.parse('the publish command prints the publish plan for "{crate_name}"'))
+def then_publish_prints_plan(cli_run: dict[str, typ.Any], crate_name: str) -> None:
+ """Assert that the publish command emits a publication plan summary."""
+ assert cli_run["returncode"] == 0
+ workspace = cli_run["workspace"]
+ lines = [line.strip() for line in cli_run["stdout"].splitlines() if line.strip()]
+ assert lines[0] == f"Publish plan for {workspace}"
+ assert "Strip patch strategy: all" in lines[1]
+ assert f"- {crate_name} @ 0.1.0" in lines
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding BDD steps for cases where no crates are publishable.
Add a test scenario where all crates are skipped, and check that the CLI outputs 'Crates to publish: none'.
```suggestion
@then(parsers.parse('the publish command prints the publish plan for "{crate_name}"'))
def then_publish_prints_plan(cli_run: dict[str, typ.Any], crate_name: str) -> None:
"""Assert that the publish command emits a publication plan summary."""
assert cli_run["returncode"] == 0
workspace = cli_run["workspace"]
lines = [line.strip() for line in cli_run["stdout"].splitlines() if line.strip()]
assert lines[0] == f"Publish plan for {workspace}"
assert "Strip patch strategy: all" in lines[1]
assert f"- {crate_name} @ 0.1.0" in lines
@then('the publish command prints that no crates are publishable')
def then_publish_prints_none(cli_run: dict[str, typ.Any]) -> None:
"""Assert that the publish command emits 'Crates to publish: none' when no crates are publishable."""
assert cli_run["returncode"] == 0
lines = [line.strip() for line in cli_run["stdout"].splitlines() if line.strip()]
assert any("Crates to publish: none" in line for line in lines)
```
</issue_to_address>
### Comment 2
<location> `tests/bdd/steps/test_publish_steps.py:58-73` </location>
<code_context>
+ 'the publish command reports configuration-skipped crate "{crate_name}"'
+ )
+)
+def then_publish_reports_configuration_skip(
+ cli_run: dict[str, typ.Any], crate_name: str
+) -> None:
+ """Assert the publish plan lists ``crate_name`` under configuration skips."""
+ lines = _publish_plan_lines(cli_run)
+ assert "Skipped via publish.exclude:" in lines
+ section_index = lines.index("Skipped via publish.exclude:")
+ skipped = lines[section_index + 1 :]
+ assert f"- {crate_name}" in skipped
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test for multiple configuration-skipped crates.
Testing with multiple crates in publish.exclude will verify that all are correctly listed as skipped in the CLI output.
```suggestion
@then(
parsers.parse(
'the publish command reports configuration-skipped crate "{crate_name}"'
)
)
def then_publish_reports_configuration_skip(
cli_run: dict[str, typ.Any], crate_name: str
) -> None:
"""Assert the publish plan lists ``crate_name`` under configuration skips."""
lines = _publish_plan_lines(cli_run)
assert "Skipped via publish.exclude:" in lines
section_index = lines.index("Skipped via publish.exclude:")
skipped = lines[section_index + 1 :]
assert f"- {crate_name}" in skipped
@then(
parsers.parse(
'the publish command reports configuration-skipped crates {crate_names}'
)
)
def then_publish_reports_multiple_configuration_skips(
cli_run: dict[str, typ.Any], crate_names: str
) -> None:
"""Assert the publish plan lists all specified crate names under configuration skips."""
lines = _publish_plan_lines(cli_run)
assert "Skipped via publish.exclude:" in lines
section_index = lines.index("Skipped via publish.exclude:")
skipped = lines[section_index + 1 :]
for crate_name in [name.strip() for name in crate_names.split(",")]:
assert f"- {crate_name}" in skipped
```
</issue_to_address>
### Comment 3
<location> `tests/bdd/steps/test_bump_steps.py:47-53` </location>
<code_context>
+ return _run_cli(repo_root, workspace_directory, "bump", version, "--dry-run")
+
+
+@then(parsers.parse('the bump command reports manifest updates for "{version}"'))
+def then_command_reports_workspace(cli_run: dict[str, typ.Any], version: str) -> None:
+ """Assert that the bump command reports the updated manifests."""
+ assert cli_run["returncode"] == 0
+ stdout = cli_run["stdout"]
+ assert "Updated version to " in stdout
+ assert version in stdout
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding negative tests for invalid version strings.
Please add BDD steps to test how the CLI responds to invalid version strings, confirming that errors are reported appropriately.
</issue_to_address>
### Comment 4
<location> `lading/commands/publish.py:36` </location>
<code_context>
+ return tuple(crate.name for crate in self.publishable)
+
+
+def plan_publication(
+ workspace: WorkspaceGraph,
+ configuration: LadingConfig,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add unit and behavioural tests for the new publication planning logic.
The new `plan_publication` function and related helpers implement non-trivial logic for determining publishable crates and exclusions. You must add both unit and behavioural tests to verify correct behaviour, including edge cases for manifest and configuration exclusions.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 5
<location> `lading/commands/publish.py:186` </location>
<code_context>
+ raise WorkspaceModelError(message) from exc
def run(
</code_context>
<issue_to_address>
**issue (review_instructions):** Add tests for the new publish command output and planning behaviour.
The `run` function now produces a publication plan and formatted output. You must add behavioural and unit tests to ensure the output matches expectations for various workspace and configuration scenarios.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 6
<location> `docs/usage-guide.md:127` </location>
<code_context>
-count, and returns successfully. Publication planning and execution will arrive
-in later phases of the roadmap.
+`publish` now produces a publication plan for the workspace. The command reads
+`publish.exclude` from `lading.toml`, honours any crate manifests that declare
+`publish = false`, and prints a structured summary listing the crates that will
+be published. Additional sections document crates skipped by manifest flags or
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The word 'honours' uses en-GB spelling; en-oxendic spelling is 'honors'.
Please update 'honours' to 'honors' to match en-oxendic spelling conventions.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 7
<location> `tests/bdd/steps/fixtures.py:402` </location>
<code_context>
publish=False if not publishable else None,
</code_context>
<issue_to_address>
**suggestion (code-quality):** Swap if/else branches of if expression to remove negation ([`swap-if-expression`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/swap-if-expression))
```suggestion
publish=None if publishable else False,
```
<br/><details><summary>Explanation</summary>Negated conditions are more difficult to read than positive ones, so it is best
to avoid them where we can. By swapping the `if` and `else` conditions around we
can invert the condition and make it positive.
</details>
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
SourceryAI
left a comment
SourceryAI
left a comment
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `Makefile:76` </location>
<code_context>
nixie: $(NIXIE) ## Validate Mermaid diagrams
- find . -type f -name '*.md' \
- -not -path './.venv/*' -print0 | xargs -0 $(NIXIE)
+ nixie --no-sandbox
test: build uv pytest ## Run tests
</code_context>
<issue_to_address>
**🚨 question (security):** Switching to nixie --no-sandbox may reduce validation isolation.
Disabling sandboxing may expose the validation process to security risks if files are untrusted. Confirm that this is necessary and that all inputs are safe.
</issue_to_address>
### Comment 2
<location> `tests/bdd/steps/test_publish_steps.py:39-41` </location>
<code_context>
+ assert f"- {crate_name} @ 0.1.0" in lines
+
+
+def _publish_plan_lines(cli_run: dict[str, typ.Any]) -> list[str]:
+ """Return trimmed publish plan output lines for ``cli_run``."""
+ return [line.strip() for line in cli_run["stdout"].splitlines() if line.strip()]
+
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a BDD step for the case where no crates are publishable.
Please add a BDD scenario to verify the CLI output when no crates are publishable, ensuring this edge case is handled clearly.
Suggested implementation:
```python
def then_publish_prints_plan(cli_run: dict[str, typ.Any], crate_name: str) -> None:
"""Assert that the publish command emits a publication plan summary."""
assert cli_run["returncode"] == 0
workspace = cli_run["workspace"]
lines = [line.strip() for line in cli_run["stdout"].splitlines() if line.strip()]
assert lines[0] == f"Publish plan for {workspace}"
assert "Strip patch strategy: all" in lines[1]
assert f"- {crate_name} @ 0.1.0" in lines
@then(parsers.parse('no crates are publishable'))
def then_publish_prints_no_crates_publishable(cli_run: dict[str, typ.Any]) -> None:
"""Assert that the publish command emits a message when no crates are publishable."""
assert cli_run["returncode"] == 0
lines = [line.strip() for line in cli_run["stdout"].splitlines() if line.strip()]
# Adjust the expected message to match your CLI's actual output
assert any(
"No crates are publishable" in line or "No crates to publish" in line
for line in lines
), f"Expected a message indicating no crates are publishable, got: {lines}"
```
You will need to add a corresponding BDD scenario in your feature file (e.g., `publish.feature`) that uses the step: `Then no crates are publishable`.
Make sure your CLI emits a clear message like "No crates are publishable" or "No crates to publish" when this edge case occurs.
Adjust the expected message in the assertion if your CLI uses different wording.
</issue_to_address>
### Comment 3
<location> `lading/commands/publish.py:115` </location>
<code_context>
+ return [header, *formatted_items]
+
+
+def _format_plan(
+ plan: PublishPlan, *, strip_patches: config_module.StripPatchesSetting
+) -> str:
</code_context>
<issue_to_address>
**issue (complexity):** Consider inlining section formatting in _format_plan and removing helper indirection in run for improved clarity.
```suggestion
# Drop `_format_section` and inline per‐section formatting in `_format_plan`
# (also removes the generic `T` and `empty_lines` hack)
-def _format_plan(
- plan: PublishPlan, *, strip_patches: config_module.StripPatchesSetting
-) -> str:
- lines = [
- f"Publish plan for {plan.workspace_root}",
- f"Strip patch strategy: {strip_patches}",
- ]
-
- lines.extend(
- _format_section(
- plan.publishable,
- header=f"Crates to publish ({len(plan.publishable)}):",
- item_formatter=lambda crate: f"- {crate.name} @ {crate.version}",
- empty_lines=("Crates to publish: none",),
- )
- )
- # … other sections …
- return "\n".join(lines)
+def _format_plan(
+ plan: PublishPlan,
+ *,
+ strip_patches: config_module.StripPatchesSetting
+) -> str:
+ lines: list[str] = [
+ f"Publish plan for {plan.workspace_root}",
+ f"Strip patch strategy: {strip_patches}",
+ ]
+
+ # Crates to publish
+ if plan.publishable:
+ lines.append(f"Crates to publish ({len(plan.publishable)}):")
+ lines.extend(f"- {c.name} @ {c.version}" for c in plan.publishable)
+ else:
+ lines.append("Crates to publish: none")
+
+ # Skipped (publish = false)
+ if plan.skipped_manifest:
+ lines.append("Skipped (publish = false):")
+ lines.extend(f"- {c.name}" for c in plan.skipped_manifest)
+
+ # Skipped via configuration
+ if plan.skipped_configuration:
+ lines.append("Skipped via publish.exclude:")
+ lines.extend(f"- {c.name}" for c in plan.skipped_configuration)
+
+ # Missing exclusions
+ if plan.missing_configuration_exclusions:
+ lines.append("Configured exclusions not found in workspace:")
+ lines.extend(f"- {name}" for name in plan.missing_configuration_exclusions)
+
+ return "\n".join(lines)
```
```suggestion
# Inline the two `_ensure_*` helpers directly in `run` to remove indirection
-def run(
- workspace_root: Path,
- configuration: LadingConfig | None = None,
- workspace: WorkspaceGraph | None = None,
-) -> str:
- root = normalise_workspace_root(workspace_root)
- active_configuration = _ensure_configuration(configuration, root)
- active_workspace = _ensure_workspace(workspace, root)
- plan = plan_publication(active_workspace, active_configuration, workspace_root=root)
- return _format_plan(plan, strip_patches=active_configuration.publish.strip_patches)
+def run(
+ workspace_root: Path,
+ configuration: LadingConfig | None = None,
+ workspace: WorkspaceGraph | None = None,
+) -> str:
+ root = normalise_workspace_root(workspace_root)
+
+ # load or fetch configuration
+ if configuration is None:
+ try:
+ configuration = config_module.current_configuration()
+ except config_module.ConfigurationNotLoadedError:
+ configuration = config_module.load_configuration(root)
+
+ # load or reuse workspace
+ if workspace is None:
+ from lading.workspace import load_workspace, WorkspaceModelError
+ try:
+ workspace = load_workspace(root)
+ except FileNotFoundError as e:
+ raise WorkspaceModelError(f"Workspace root not found: {root}") from e
+
+ plan = plan_publication(workspace, configuration, workspace_root=root)
+ return _format_plan(plan, strip_patches=configuration.publish.strip_patches)
```
</issue_to_address>
### Comment 4
<location> `lading/commands/publish.py:36` </location>
<code_context>
+ return tuple(crate.name for crate in self.publishable)
+
+
+def plan_publication(
+ workspace: WorkspaceGraph,
+ configuration: LadingConfig,
</code_context>
<issue_to_address>
**issue (review_instructions):** Add unit and behavioural tests for the new publication planning logic.
The new functions and logic for publication planning (e.g., plan_publication, PublishPlan, _format_plan, etc.) require both unit and behavioural tests to verify correct behaviour and edge cases. No new tests are present in the diff.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 5
<location> `lading/commands/publish.py:186` </location>
<code_context>
+ raise WorkspaceModelError(message) from exc
def run(
</code_context>
<issue_to_address>
**issue (review_instructions):** Add tests for the new run function implementation.
The run function was substantially changed to implement publication planning. This new behaviour must be covered by both unit and behavioural tests, but no new tests are present in the diff.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 6
<location> `docs/usage-guide.md:127` </location>
<code_context>
-count, and returns successfully. Publication planning and execution will arrive
-in later phases of the roadmap.
+`publish` now produces a publication plan for the workspace. The command reads
+`publish.exclude` from `lading.toml`, honours any crate manifests that declare
+`publish = false`, and prints a structured summary listing the crates that will
+be published. Additional sections document crates skipped by manifest flags or
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The word 'honours' uses en-GB spelling; en-oxendic spelling is preferred ('honors').
Please update 'honours' to 'honors' to match the en-oxendic spelling convention specified in the review instructions.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Use en-oxendic (-ize / -yse / -our) spelling and grammar.
</details>
</issue_to_address>
### Comment 7
<location> `docs/usage-guide.md:148` </location>
<code_context>
+```
+
+When the configuration excludes additional crates, or a manifest sets
+`publish = false`, the plan prints dedicated sections so the operator can see
+why those crates were skipped.
+
</code_context>
<issue_to_address>
**suggestion (review_instructions):** The phrase 'so the operator can see why those crates were skipped' uses 2nd person construction; rephrase to avoid this.
Consider rephrasing to 'so the reasons for skipping crates are visible to the operator.'
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*.md`
**Instructions:**
Avoid 2nd person or 1st person pronouns ("I", "you", "we")
</details>
</issue_to_address>
### Comment 8
<location> `tests/bdd/steps/fixtures.py:402` </location>
<code_context>
publish=False if not publishable else None,
</code_context>
<issue_to_address>
**suggestion (code-quality):** Swap if/else branches of if expression to remove negation ([`swap-if-expression`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/swap-if-expression))
```suggestion
publish=None if publishable else False,
```
<br/><details><summary>Explanation</summary>Negated conditions are more difficult to read than positive ones, so it is best
to avoid them where we can. By swapping the `if` and `else` conditions around we
can invert the condition and make it positive.
</details>
</issue_to_address>Hi @leynos! 👋
Thanks for trying out Sourcery by commenting with @sourcery-ai review! 🚀
Install the sourcery-ai bot to get automatic code reviews on every pull request ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.| def _publish_plan_lines(cli_run: dict[str, typ.Any]) -> list[str]: | ||
| """Return trimmed publish plan output lines for ``cli_run``.""" | ||
| return [line.strip() for line in cli_run["stdout"].splitlines() if line.strip()] |
There was a problem hiding this comment.
suggestion (testing): Consider adding a BDD step for the case where no crates are publishable.
Please add a BDD scenario to verify the CLI output when no crates are publishable, ensuring this edge case is handled clearly.
Suggested implementation:
def then_publish_prints_plan(cli_run: dict[str, typ.Any], crate_name: str) -> None:
"""Assert that the publish command emits a publication plan summary."""
assert cli_run["returncode"] == 0
workspace = cli_run["workspace"]
lines = [line.strip() for line in cli_run["stdout"].splitlines() if line.strip()]
assert lines[0] == f"Publish plan for {workspace}"
assert "Strip patch strategy: all" in lines[1]
assert f"- {crate_name} @ 0.1.0" in lines
@then(parsers.parse('no crates are publishable'))
def then_publish_prints_no_crates_publishable(cli_run: dict[str, typ.Any]) -> None:
"""Assert that the publish command emits a message when no crates are publishable."""
assert cli_run["returncode"] == 0
lines = [line.strip() for line in cli_run["stdout"].splitlines() if line.strip()]
# Adjust the expected message to match your CLI's actual output
assert any(
"No crates are publishable" in line or "No crates to publish" in line
for line in lines
), f"Expected a message indicating no crates are publishable, got: {lines}"You will need to add a corresponding BDD scenario in your feature file (e.g., publish.feature) that uses the step: Then no crates are publishable.
Make sure your CLI emits a clear message like "No crates are publishable" or "No crates to publish" when this edge case occurs.
Adjust the expected message in the assertion if your CLI uses different wording.
| return [header, *formatted_items] | ||
|
|
||
|
|
||
| def _format_plan( |
There was a problem hiding this comment.
issue (complexity): Consider inlining section formatting in _format_plan and removing helper indirection in run for improved clarity.
| def _format_plan( | |
| # Drop `_format_section` and inline per‐section formatting in `_format_plan` | |
| # (also removes the generic `T` and `empty_lines` hack) | |
| -def _format_plan( | |
| - plan: PublishPlan, *, strip_patches: config_module.StripPatchesSetting | |
| -) -> str: | |
| - lines = [ | |
| - f"Publish plan for {plan.workspace_root}", | |
| - f"Strip patch strategy: {strip_patches}", | |
| - ] | |
| - | |
| - lines.extend( | |
| - _format_section( | |
| - plan.publishable, | |
| - header=f"Crates to publish ({len(plan.publishable)}):", | |
| - item_formatter=lambda crate: f"- {crate.name} @ {crate.version}", | |
| - empty_lines=("Crates to publish: none",), | |
| - ) | |
| - ) | |
| - # … other sections … | |
| - return "\n".join(lines) | |
| +def _format_plan( | |
| + plan: PublishPlan, | |
| + *, | |
| + strip_patches: config_module.StripPatchesSetting | |
| +) -> str: | |
| + lines: list[str] = [ | |
| + f"Publish plan for {plan.workspace_root}", | |
| + f"Strip patch strategy: {strip_patches}", | |
| + ] | |
| + | |
| + # Crates to publish | |
| + if plan.publishable: | |
| + lines.append(f"Crates to publish ({len(plan.publishable)}):") | |
| + lines.extend(f"- {c.name} @ {c.version}" for c in plan.publishable) | |
| + else: | |
| + lines.append("Crates to publish: none") | |
| + | |
| + # Skipped (publish = false) | |
| + if plan.skipped_manifest: | |
| + lines.append("Skipped (publish = false):") | |
| + lines.extend(f"- {c.name}" for c in plan.skipped_manifest) | |
| + | |
| + # Skipped via configuration | |
| + if plan.skipped_configuration: | |
| + lines.append("Skipped via publish.exclude:") | |
| + lines.extend(f"- {c.name}" for c in plan.skipped_configuration) | |
| + | |
| + # Missing exclusions | |
| + if plan.missing_configuration_exclusions: | |
| + lines.append("Configured exclusions not found in workspace:") | |
| + lines.extend(f"- {name}" for name in plan.missing_configuration_exclusions) | |
| + | |
| + return "\n".join(lines) |
| def _format_plan( | |
| # Inline the two `_ensure_*` helpers directly in `run` to remove indirection | |
| -def run( | |
| - workspace_root: Path, | |
| - configuration: LadingConfig | None = None, | |
| - workspace: WorkspaceGraph | None = None, | |
| -) -> str: | |
| - root = normalise_workspace_root(workspace_root) | |
| - active_configuration = _ensure_configuration(configuration, root) | |
| - active_workspace = _ensure_workspace(workspace, root) | |
| - plan = plan_publication(active_workspace, active_configuration, workspace_root=root) | |
| - return _format_plan(plan, strip_patches=active_configuration.publish.strip_patches) | |
| +def run( | |
| + workspace_root: Path, | |
| + configuration: LadingConfig | None = None, | |
| + workspace: WorkspaceGraph | None = None, | |
| +) -> str: | |
| + root = normalise_workspace_root(workspace_root) | |
| + | |
| + # load or fetch configuration | |
| + if configuration is None: | |
| + try: | |
| + configuration = config_module.current_configuration() | |
| + except config_module.ConfigurationNotLoadedError: | |
| + configuration = config_module.load_configuration(root) | |
| + | |
| + # load or reuse workspace | |
| + if workspace is None: | |
| + from lading.workspace import load_workspace, WorkspaceModelError | |
| + try: | |
| + workspace = load_workspace(root) | |
| + except FileNotFoundError as e: | |
| + raise WorkspaceModelError(f"Workspace root not found: {root}") from e | |
| + | |
| + plan = plan_publication(workspace, configuration, workspace_root=root) | |
| + return _format_plan(plan, strip_patches=configuration.publish.strip_patches) |
| count, and returns successfully. Publication planning and execution will arrive | ||
| in later phases of the roadmap. | ||
| `publish` now produces a publication plan for the workspace. The command reads | ||
| `publish.exclude` from `lading.toml`, honours any crate manifests that declare |
There was a problem hiding this comment.
suggestion (review_instructions): The word 'honours' uses en-GB spelling; en-oxendic spelling is preferred ('honors').
Please update 'honours' to 'honors' to match the en-oxendic spelling convention specified in the review instructions.
Review instructions:
Path patterns: **/*.md
Instructions:
Use en-oxendic (-ize / -yse / -our) spelling and grammar.
| ``` | ||
|
|
||
| When the configuration excludes additional crates, or a manifest sets | ||
| `publish = false`, the plan prints dedicated sections so the operator can see |
There was a problem hiding this comment.
suggestion (review_instructions): The phrase 'so the operator can see why those crates were skipped' uses 2nd person construction; rephrase to avoid this.
Consider rephrasing to 'so the reasons for skipping crates are visible to the operator.'
Review instructions:
Path patterns: **/*.md
Instructions:
Avoid 2nd person or 1st person pronouns ("I", "you", "we")
| _build_package_metadata( | ||
| name, | ||
| manifest_path, | ||
| publish=False if not publishable else None, |
There was a problem hiding this comment.
suggestion (code-quality): Swap if/else branches of if expression to remove negation (swap-if-expression)
| publish=False if not publishable else None, | |
| publish=None if publishable else False, |
Explanation
Negated conditions are more difficult to read than positive ones, so it is bestto avoid them where we can. By swapping the
if and else conditions around wecan invert the condition and make it positive.
|
@sourcery-ai resolve |
2 participants
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68f62fc6b8c883229ed9ccadb82a5411
Summary by Sourcery
Implement crate publication planning in the
publishcommand by filtering on manifest and configuration excludes, rendering a structured plan in CLI output, and updating docs, Makefile and tests to reflect the new behavior.New Features:
Enhancements:
Documentation:
Tests:
Summary by CodeRabbit
New Features
Documentation
Tests
Chores