Add configurable documentation updates to bump command - #15
Conversation
WalkthroughAdds configurable documentation-aware bumping: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant CLI as CLI
participant Bump as bump.run()
participant Manifest as Manifest Updater
participant DocResolver as Doc Resolver
participant DocUpdater as Doc Updater
participant Formatter as Result Formatter
CLI->>Bump: invoke bump to version X
Bump->>Manifest: compute & apply manifest changes
Manifest-->>Bump: changed_manifests
Bump->>DocResolver: expand bump.documentation.globs
DocResolver-->>Bump: documentation_paths
Bump->>DocUpdater: parse Markdown TOML fences\n(update [package]/[workspace.package]/deps)
DocUpdater-->>Bump: changed_documents
Bump->>Formatter: format summary with\nmanifests + documentation
Formatter-->>CLI: print manifest + documentation summary
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
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 (1)**/*.py📄 CodeRabbit inference engine (.rules/python-00.md)
Files:
⏰ 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 (3)
Comment |
|
@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 +552 to +585 def _update_toml_snippet_versions(
snippet: str,
dependency_targets: typ.Collection[str],
target_version: str,
) -> tuple[str, bool]:
"""Return a TOML snippet with dependency versions rewritten."""
try:
document = parse_toml(snippet)
except TOMLKitError:
return snippet, False
changed = False
if _assign_version(_select_table(document, ("package",)), target_version):
changed = True
if _assign_version(
_select_table(document, ("workspace", "package")), target_version
):
changed = True
if dependency_targets:
for section in ("dependencies", "dev-dependencies", "build-dependencies"):
table = _select_table(document, (section,))
if table is None:
continue
if _update_dependency_table(table, dependency_targets, target_version):
changed = True
if not changed:
return snippet, False
suffix_match = re.search(r"((?:\r?\n)*)$", snippet)
newline_suffix = suffix_match.group(1) if suffix_match else ""
rendered = document.as_string().rstrip("\r\n")
return (f"{rendered}{newline_suffix}" if newline_suffix else rendered, True)❌ 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 def _format_result_message(
changed_manifests: typ.Sequence[Path],
changed_documents: typ.Sequence[Path],❌ 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.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
lading/commands/bump.py (1)
558-591: TOML snippet update logic is functional and well-structured.The function correctly handles all version update scenarios with appropriate error handling. The trailing newline preservation (lines 588-591) maintains formatting consistency.
The PR objectives note that static analysis flagged the cyclomatic complexity (9, at threshold). The suggested optional refactor would extract the dependency-section loop (lines 577-583) into a helper function
_update_toml_snippet_dependencies. This would reduce complexity to ~5 while preserving behavior, but the current implementation remains readable and maintainable.
📜 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 (2)
lading/commands/bump.py(7 hunks)tests/unit/test_bump_command_internals.py(4 hunks)
🧰 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/unit/test_bump_command_internals.pylading/commands/bump.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/unit/test_bump_command_internals.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/unit/test_bump_command_internals.py
🧬 Code graph analysis (2)
tests/unit/test_bump_command_internals.py (2)
tests/helpers/workspace_builders.py (1)
_make_config(132-144)lading/commands/bump.py (2)
_format_result_message(176-210)BumpChanges(60-64)
lading/commands/bump.py (4)
lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/config.py (2)
LadingConfig(109-130)DocumentationConfig(36-55)lading/workspace/models.py (2)
WorkspaceCrate(31-41)WorkspaceGraph(44-53)lading/cli.py (1)
bump(198-218)
⏰ 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
🔇 Additional comments (10)
tests/unit/test_bump_command_internals.py (2)
226-226: LGTM!The updated call correctly uses the keyword-only
excludeparameter, aligning with the refactored_make_configsignature.
242-289: Excellent test coverage for BumpChanges refactor.The test correctly validates all scenarios: empty changes, manifests-only, dry-run, and the new combined manifests+documents case. The assertions properly verify the formatted output including the "(documentation)" suffix for documentation files.
lading/commands/bump.py (8)
14-30: LGTM!The new imports for Markdown and TOML processing are appropriate and properly organized. Type-checking imports are correctly guarded with
TYPE_CHECKING, and runtime placeholders are provided as per guidelines.
59-65: LGTM!The
BumpChangesdataclass effectively groups related file collections, reducing the parameter count in_format_result_messagewhile maintaining clarity. The use offrozen=Trueensures immutability as per guidelines.
118-142: LGTM!The documentation processing is cleanly integrated into the main bump flow. The ordering and result formatting correctly handle both manifests and documentation files, maintaining consistency with the existing manifest-handling logic.
176-210: LGTM!The refactored signature successfully addresses the parameter-count issue by bundling related collections in
BumpChanges. The function now has 4 parameters (within guidelines) while maintaining clarity. The logic correctly handles all change combinations and provides clear output formatting.
436-449: LGTM!The implementation correctly resolves glob patterns and deduplicates results using a dict as an ordered set (idiomatic for Python 3.7+). File filtering and tuple return maintain correctness and immutability.
452-515: LGTM!The documentation update workflow is well-structured:
- Atomic writes ensure safety during updates
- The Markdown fence replacement logic correctly reconstructs documents by splicing updated fences into the original token stream
- The
nonlocalpattern in_rewrite_markdown_toml_fencesappropriately tracks changes across the closure- Error handling via the
TOMLKitErrorcatch ensures malformed TOML snippets are left unchanged
518-556: LGTM!The fence processing helpers are well-designed:
_token_matches_languagehandles edge cases (empty info, case-insensitivity)_render_fencecorrectly preserves formatting (indentation, trailing newlines) while applying transformations- Each helper has a single, clear responsibility
643-663: LGTM!The parameter rename from
manifest_pathtofile_pathappropriately generalizes the function for use with both manifests and documentation files. All references are consistently updated, and the atomic write semantics remain unchanged.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
lading/commands/bump.py (2)
59-65: Addslots=Truefor internal dataclass.The
BumpChangesdataclass is used internally and should includeslots=Truefor improved memory efficiency and faster attribute access, per coding guidelines.Apply this diff:
-@dc.dataclass(frozen=True) +@dc.dataclass(frozen=True, slots=True) class BumpChanges: """Collection of files altered by a bump run."""
436-449: Consider usingset[Path]for deduplication.The current implementation uses
dict[Path, None]to deduplicate resolved paths. While correct, aset[Path]would be clearer and more idiomatic for this purpose.Apply this diff:
- resolved: dict[Path, None] = {} + resolved: set[Path] = set() for pattern in patterns: for candidate in workspace_root.glob(pattern): if candidate.is_file(): - resolved.setdefault(candidate, None) + resolved.add(candidate) return tuple(resolved)
📜 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/bump.py(7 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/bump.py
🧬 Code graph analysis (1)
lading/commands/bump.py (4)
lading/utils/path.py (1)
normalise_workspace_root(10-16)lading/config.py (2)
LadingConfig(109-130)DocumentationConfig(36-55)lading/workspace/models.py (2)
WorkspaceCrate(31-41)WorkspaceGraph(44-53)lading/cli.py (1)
bump(198-218)
🔇 Additional comments (12)
lading/commands/bump.py (12)
14-14: LGTM!The new imports for
MarkdownIt,TOMLKitError, andTokenare appropriate for the documentation fence processing functionality. The TYPE_CHECKING guard correctly avoids runtime import overhead.Also applies to: 17-17, 24-24, 30-30
118-142: LGTM!The documentation workflow integration is well-structured: resolves targets from configuration, updates files, orders results, and groups changes in
BumpChangesfor consistent formatting.
176-210: LGTM! Refactor successfully reduces parameter count.The function signature now accepts a grouped
BumpChangesparameter instead of separate manifest and document sequences, reducing the argument count to 4 as intended. The logic correctly handles both manifest and documentation file changes with appropriate labeling.
452-472: LGTM!The function correctly processes documentation files, respects the
dry_runflag, and uses atomic writes. File I/O errors will propagate to the caller, which is appropriate for CLI tooling.
475-493: LGTM!The nested
_applyfunction withnonlocal changedis an appropriate pattern for tracking modifications across multiple fence replacements. The logic is clear and correctly structured.
496-515: LGTM!The line-based reconstruction algorithm correctly preserves original formatting and line endings while selectively transforming fenced code blocks. The use of
keepends=Trueandtoken.mapchecks ensure correctness.
518-524: LGTM!The function safely handles edge cases (empty
token.info) and correctly performs case-insensitive language matching for fence blocks.
527-549: LGTM!The function correctly preserves fence formatting, indentation, and trailing newlines while applying the transformation. The defensive
ValueErrorfor missingtoken.mapis appropriate.
552-555: LGTM!Simple, correct helper for extracting fence indentation with appropriate edge-case handling.
558-574: LGTM! Complexity reduction helper.This helper function successfully extracts the dependency-section loop from
_update_toml_snippet_versions, reducing its complexity while maintaining clear, testable logic.
577-605: LGTM! Complexity reduced via helper extraction.The extraction of
_update_toml_snippet_dependenciessuccessfully reduces the cyclomatic complexity of this function from the previously flagged level of 9. The logic remains clear and preserves TOML formatting trivia and trailing newlines correctly.
657-674: LGTM! Appropriate generalization.Renaming the parameter from
manifest_pathtofile_pathcorrectly reflects the function's broader usage for both manifests and documentation files. All references are consistently updated.
|
@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 +552 to +585 def _update_toml_snippet_versions(
snippet: str,
dependency_targets: typ.Collection[str],
target_version: str,
) -> tuple[str, bool]:
"""Return a TOML snippet with dependency versions rewritten."""
try:
document = parse_toml(snippet)
except TOMLKitError:
return snippet, False
changed = False
if _assign_version(_select_table(document, ("package",)), target_version):
changed = True
if _assign_version(
_select_table(document, ("workspace", "package")), target_version
):
changed = True
if dependency_targets:
for section in ("dependencies", "dev-dependencies", "build-dependencies"):
table = _select_table(document, (section,))
if table is None:
continue
if _update_dependency_table(table, dependency_targets, target_version):
changed = True
if not changed:
return snippet, False
suffix_match = re.search(r"((?:\r?\n)*)$", snippet)
newline_suffix = suffix_match.group(1) if suffix_match else ""
rendered = document.as_string().rstrip("\r\n")
return (f"{rendered}{newline_suffix}" if newline_suffix else rendered, True)❌ 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 def _format_result_message(
changed_manifests: typ.Sequence[Path],
changed_documents: typ.Sequence[Path],❌ New issue: Excess Number of Function Arguments |
|
Tip For best results, initiate chat on the files or code changes. @leynos, I already provided a comprehensive fix for this cyclomatic complexity issue in my previous response above. To summarize, I suggested:
This refactoring reduces the complexity from 9 to 5 while maintaining identical behavior. The complete code and AI coding agent prompt are in my earlier response. Would you like me to clarify any aspect of the suggested fix, or would you prefer a different approach to reducing the complexity? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1882 🏁 Script executed: Length of output: 413 @leynos: The fix for this issue has already been applied! 🎉 Looking at the current code in
@dc.dataclass(frozen=True)
class BumpChanges:
"""Collection of files altered by a bump run."""
manifests: typ.Sequence[Path] = ()
documents: typ.Sequence[Path] = ()
def _format_result_message(
changes: BumpChanges,
target_version: str,
*,
dry_run: bool,
workspace_root: Path,
) -> str:This gives us 4 arguments total (2 positional + 2 keyword-only), which meets the threshold of max 4 arguments.
return _format_result_message(
BumpChanges(manifests=ordered_manifests, documents=ordered_documents),
target_version,
dry_run=options.dry_run,
workspace_root=workspace_root,
)The CodeScene report you're seeing may be from an earlier commit or a stale analysis. The current code already implements the recommended refactoring and should pass the "Excess Number of Function Arguments" check. |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Complex Methodlading/commands/bump.py: _format_result_message What lead to degradation?_format_result_message has a cyclomatic complexity of 10, threshold = 9 Why does this problem occur?A Complex Method has a high cyclomatic complexity. The recommended threshold for the Python language is a cyclomatic complexity lower than 9. How to fix it?There are many reasons for Complex Method. Sometimes, another design approach is beneficial such as a) modeling state using an explicit state machine rather than conditionals, or b) using table lookup rather than long chains of logic. In other scenarios, the function can be split using EXTRACT FUNCTION. Just make sure you extract natural and cohesive functions. Complex Methods can also be addressed by identifying complex conditional expressions and then using the DECOMPOSE CONDITIONAL refactoring. Helpful refactoring examplesTo get a general understanding of what this code health issue looks like - and how it might be addressed - we have prepared some diffs for illustrative purposes. SAMPLE# complex_method.js
function postItem(item) {
if (!item.id) {
- if (item.x != null && item.y != null) {
- post(item);
- } else {
- throw Error("Item must have x and y");
- }
+ // extract a separate function for creating new item
+ postNew(item);
} else {
- if (item.x < 10 && item.y > 25) {
- put(item);
- } else {
- throw Error("Item must have an x and y value between 10 and 25");
- }
+ // and one for updating existing items
+ updateItem(item);
}
}
+
+function postNew(item) {
+ validateNew(item);
+ post(item);
+}
+
+function updateItem(item) {
+ validateUpdate(item);
+ put(item);
+}
+ |
|
@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix: Code Duplicationlading/config.py: What lead to degradation?The module contains 2 functions with similar structure: BumpConfig.from_mapping,PublishConfig.from_mapping Why does this problem occur?Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health. How to fix it?A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 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/config.py(4 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/config.py
🪛 GitHub Actions: CI
lading/config.py
[error] 138-140: ruff: D413 Missing blank line after last section ("Raises"); add a blank line after the Raises section. 1 fixable with the --fix option.
⏰ 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
🔇 Additional comments (5)
lading/config.py (5)
35-52: LGTM! Clean implementation of DocumentationConfig.The implementation follows all coding guidelines: frozen dataclass with slots, precise type annotations, proper validation via the centralized helper, and consistent pattern matching with other config classes.
59-59: LGTM! Clean integration of DocumentationConfig into BumpConfig.The documentation field is properly integrated with a default_factory, and the from_mapping method correctly validates keys and delegates to DocumentationConfig.from_mapping.
Also applies to: 66-72
88-92: LGTM! Centralized validation applied.The refactoring to use
_validate_mapping_keyssuccessfully eliminates the code duplication flagged in previous reviews while maintaining the same validation behavior.
110-110: LGTM! Top-level validation added.Correctly applies centralized validation to the top-level configuration sections.
66-72: Code duplication successfully eliminated.The introduction of
_validate_mapping_keyseffectively addresses the code duplication flagged by CodeScene betweenBumpConfig.from_mappingandPublishConfig.from_mapping. The validation logic is now centralized and reused across all config classes.Based on learnings
Also applies to: 88-92
Summary
lading.tomland document how to use itbumpcommand to rewrite Markdown TOML fences alongside manifests and surface documentation paths in the summaryTesting
https://chatgpt.com/codex/tasks/task_e_68f62f0189748322b635390479d80028
Summary by CodeRabbit
New Features
Documentation
Tests