Fix lint issues in version tools - #4
Conversation
Reviewer's GuideThis PR addresses lint errors across the version bump and publish helper scripts by adjusting type and abc imports, converting docstrings to raw strings, extracting Markdown update error handling, enhancing pytest tests, and adopting modern union syntax. Sequence diagram for extracted Markdown update error handling in bump_version.pysequenceDiagram
participant main
participant _warn_on_markdown_update_failure
participant _update_markdown_versions
participant sys.stderr
main->>_warn_on_markdown_update_failure: Call with md_path, version
_warn_on_markdown_update_failure->>_update_markdown_versions: Try to update Markdown
_update_markdown_versions-->>_warn_on_markdown_update_failure: Exception (TOMLKitError, OSError, TypeError, ValueError)
_warn_on_markdown_update_failure->>sys.stderr: Print warning message
Class diagram for updated type usage in bump_version.pyclassDiagram
class bump_version {
+_is_matching_fence_token(tok: Token, lang: str) bool
+_process_fence_token(tok: Token, lines: list[str], lang: str, replace_fn: cabc.Callable[[str], str]) str
+replace_fences(md_text: str, lang: str, replace_fn: cabc.Callable[[str], str]) str
+_update_package_version(doc: cabc.MutableMapping[str, object], version: str) None
+_extract_version_prefix(entry: tomlkit.items.String | cabc.Mapping[str, object] | str | None) str
+_update_dict_dependency(entry: cabc.MutableMapping[str, object], version: str) None
+_update_string_dependency(deps: cabc.MutableMapping[str, object], dependency: str, entry: tomlkit.items.String | str, version: str) None
+_update_dependency_in_table(deps: cabc.MutableMapping[str, object], dependency: str, version: str) None
+_update_dependency_version(doc: cabc.MutableMapping[str, object], dependency: str, version: str) None
+_set_version(toml_path: Path, version: str, dependency: str | None = None, doc: cabc.MutableMapping[str, object] | None = None) None
+_warn_on_markdown_update_failure(md_path: Path, version: str) None
+main(argv: list[str]) int
}
bump_version ..> "cabc.Callable" : uses
bump_version ..> "cabc.MutableMapping" : uses
bump_version ..> "cabc.Mapping" : uses
bump_version ..> "Token" : uses
bump_version ..> "Path" : uses
bump_version ..> "tomlkit.items.String" : uses
Class diagram for updated union syntax in publish_patch.pyclassDiagram
class publish_patch {
+extract_existing_items(value: object) tuple[tuple[str, object], ...]
}
publish_patch ..> "Table" : uses
publish_patch ..> "InlineTable" : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
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. WalkthroughMakefile's typecheck target now runs Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as Developer
participant Make as Makefile (typecheck)
participant Ty as ty
participant Scripts as crate_tools/*.py
Dev->>Make: make typecheck
Make->>Ty: ty check --python-version 3.13 --extra-search-path crate_tools $(CRATE_TOOLS_SCRIPTS)
Ty-->>Make: Type-check results
Make-->>Dev: Report
sequenceDiagram
autonumber
actor User
participant BV as bump_version.py
participant TK as tomlkit
participant FS as Filesystem
participant Warn as _warn_on_markdown_update_failure
participant Log as logger
User->>BV: run update
BV->>TK: parse TOML (typed/cast)
BV->>BV: process fence tokens (raise/skip if mapping missing)
BV->>FS: write temp file
BV->>FS: Path(temp_name).replace(...) for atomic swap
alt Markdown update raises TOMLKitError
BV->>Warn: call helper
Warn->>Log: emit warning
else success
BV-->>User: files updated
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (.rules/python-00.md)
Files:
**/unittests/test_*.py📄 CodeRabbit inference engine (.rules/python-00.md)
Files:
{**/unittests/test_*.py,tests/**/*.py}📄 CodeRabbit inference engine (.rules/python-00.md)
Files:
🧬 Code graph analysis (1)crate_tools/unittests/test_bump_version.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 (26)
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.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and found some issues that need to be addressed.
- Consider introducing a type alias for repeated annotations like cabc.Mapping[str, object] to reduce verbosity and improve readability.
- Might be better to use the logging module instead of print in _warn_on_markdown_update_failure so warnings integrate with existing logging configuration.
- In test_update_markdown_versions_behavior you could parameterize the expected change directly instead of computing should_change from outcome to simplify the test logic.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider introducing a type alias for repeated annotations like cabc.Mapping[str, object] to reduce verbosity and improve readability.
- Might be better to use the logging module instead of print in _warn_on_markdown_update_failure so warnings integrate with existing logging configuration.
- In test_update_markdown_versions_behavior you could parameterize the expected change directly instead of computing should_change from outcome to simplify the test logic.
## Individual Comments
### Comment 1
<location> `crate_tools/bump_version.py:588-594` </location>
<code_context>
md_path.write_text(updated, encoding="utf-8")
+def _warn_on_markdown_update_failure(md_path: Path, version: str) -> None:
+ """Emit a warning if a markdown update fails."""
+ try:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Catching broad exceptions may mask unexpected errors.
Review whether TypeError and ValueError should be caught here, as handling them may obscure programming mistakes or unexpected input. Allowing some exceptions to propagate could improve error visibility during development.
```suggestion
try:
_update_markdown_versions(md_path, version)
except (TOMLKitError, OSError) as exc:
print(
f"Warning: Failed to update {md_path}: {exc}",
file=sys.stderr,
)
```
</issue_to_address>
### Comment 2
<location> `crate_tools/unittests/test_bump_version.py:71` </location>
<code_context>
def test_workspace_dependency_no_version_written() -> None:
+ """Skip adding explicit versions for workspace-managed dependencies."""
doc = tomlkit.parse("[dependencies]\nfoo = { workspace = true }\n")
</code_context>
<issue_to_address>
**suggestion (testing):** Missing test for markdown update failure warning.
Add a test that triggers a markdown update failure and verifies that the warning is correctly emitted to stderr.
Suggested implementation:
```python
def test_workspace_dependency_no_version_written() -> None:
"""Skip adding explicit versions for workspace-managed dependencies."""
doc = tomlkit.parse("[dependencies]\nfoo = { workspace = true }\n")
_update_dependency_version(doc, "foo", "1.2.3")
deps = doc["dependencies"]["foo"]
def test_markdown_update_failure_warning(capsys) -> None:
"""Emit a warning to stderr when markdown update fails."""
# Simulate markdown update failure by calling the function with bad input
# This assumes _update_markdown returns False or raises on failure
# Replace with actual function and failure mode as needed
try:
result = _update_markdown("bad input", "1.2.3")
except Exception:
# If the function raises, emit warning manually
print("WARNING: Failed to update markdown", file=sys.stderr)
else:
if not result:
print("WARNING: Failed to update markdown", file=sys.stderr)
captured = capsys.readouterr()
assert "WARNING: Failed to update markdown" in captured.err
```
- Ensure that `_update_markdown` is imported or available in the test file.
- Adjust the failure simulation to match the actual behavior of your markdown update function (e.g., whether it returns False or raises an exception).
- If your warning message is different, update the string in the assertion and print statement accordingly.
</issue_to_address>
### Comment 3
<location> `crate_tools/bump_version.py:586` </location>
<code_context>
md_path.write_text(updated, encoding="utf-8")
+def _warn_on_markdown_update_failure(md_path: Path, version: str) -> None:
+ """Emit a warning if a markdown update fails."""
+ try:
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for the new _warn_on_markdown_update_failure function.
The new function _warn_on_markdown_update_failure is added, but there are no corresponding behavioural or unit tests verifying its correct operation or error handling. Add tests to cover this new functionality.
<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>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
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: 1
♻️ Duplicate comments (1)
crate_tools/bump_version.py (1)
598-607: Broad exception catch may mask programming errors.The helper function
_warn_on_markdown_update_failurecatchesTypeErrorandValueErrorin addition toTOMLKitErrorandOSError. While this prevents crashes, it may hide programming mistakes or unexpected input issues that should be surfaced during development.Consider whether
TypeErrorandValueErrorshould be caught here. These typically indicate programming errors rather than expected failures. You might want to:
- Remove
TypeErrorandValueErrorfrom the catch list to let programming errors surface- Or, add specific handling that logs more details about what went wrong for debugging
Current code:
except (TOMLKitError, OSError, TypeError, ValueError) as exc:Safer alternative:
except (TOMLKitError, OSError) as exc:This matches the past review comment from Sourcery AI on the same concern.
📜 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 (6)
Makefile(1 hunks)crate_tools/bump_version.py(17 hunks)crate_tools/publish_patch.py(4 hunks)crate_tools/publish_workspace_dependencies.py(1 hunks)crate_tools/unittests/test_bump_version.py(10 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.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:
crate_tools/publish_workspace_dependencies.pycrate_tools/publish_patch.pycrate_tools/bump_version.pycrate_tools/unittests/test_bump_version.py
pyproject.toml
📄 CodeRabbit inference engine (.rules/python-00.md)
pyproject.toml: Enable Ruff for linting (replacing flake8, isort, pyflakes, etc.) and configure it
Use Ruff as the project formatter; let Ruff handle all formatting
Configure tools (Ruff, Pyright, Pytest) via pyproject.toml
Enforce strict mode in PyrightConfigure Ruff to enforce TRY, BLE, EM, LOG, N818, PERF203, and B017 in pyproject.toml
pyproject.toml: Use the PEP 621 [project] table with at least name and version defined
Include description and readme in [project]; set readme to the README file path (e.g., README.md)
Set requires-python in [project] to declare supported Python versions (e.g., >=3.10)
Specify license in [project] using license = { text = "" } or license = { file = "LICENSE" }
Provide authors with name and email in [project].authors
Use keywords and valid Trove classifiers in [project]
Declare runtime dependencies in [project].dependencies using PEP 508 specifiers
Group non-runtime deps under [project.optional-dependencies] (e.g., dev, docs)
Define CLI entry points under [project.scripts] (e.g., mycli = "pkg.cli:main")
Define GUI entry points under [project.gui-scripts] when needed
Register plugin entry points under [project.entry-points.'group.name']
Declare a build system: [build-system] requires = ["setuptools>=61.0", "wheel"], build-backend = "setuptools.build_meta"
Set [tool.uv].package = true to ensure your project is built/installed on uv sync/run
If omitting [build-system], set [tool.uv].package = true so uv still builds/installs your package
Use semantic versioning (e.g., 1.2.3) for the [project].version value
Keep build constraints minimal; omit [build-system] if you don’t need editable installs
Prefer exact or bounded dependency ranges (e.g., requests>=2.25,<3.0) to avoid unexpected major bumps
Use dynamic fields (e.g., dynamic = ["version"]) sparingly and only if the build backend supports them
Files:
pyproject.toml
**/unittests/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate unit tests with code in an unittests/ subdirectory using test_ prefix
Files:
crate_tools/unittests/test_bump_version.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:
crate_tools/unittests/test_bump_version.py
🧬 Code graph analysis (2)
crate_tools/publish_workspace_dependencies.py (1)
crate_tools/publish_patch.py (1)
apply_replacements(61-125)
crate_tools/unittests/test_bump_version.py (1)
crate_tools/bump_version.py (2)
_update_dependency_version(278-314)replace_version_in_toml(534-564)
⏰ 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 (15)
pyproject.toml (1)
27-27: LGTM! Test directory exclusion is appropriate.The exclusion of test directories from type checking aligns with the project structure where tests are colocated in
unittests/subdirectories. This is a standard practice and matches the coding guidelines.crate_tools/publish_workspace_dependencies.py (1)
13-13: LGTM! Absolute import improves clarity.The change from a relative import to an absolute, namespace-qualified import (
from crate_tools.publish_patch import ...) improves code clarity and aligns with the module being part of thecrate_toolspackage structure.crate_tools/publish_patch.py (3)
20-24: Runtime fallback for TYPE_CHECKING alias is correct.The pattern of providing a runtime fallback for the
cabcalias whenTYPE_CHECKINGisFalseis correct. The cast to"type[object]"ensures that at runtime,cabccan be used as a namespace-like object without importing the actualcollections.abcmodule.
175-182: Type checking with runtime validation is appropriate.The addition of runtime type checking to ensure
section_itemis either aTableorInlineTablebefore casting toMutableMappingis correct. RaisingSystemExitwhen the check fails maintains the existing error handling pattern.
265-273: Cast to bypass tomlkit's incomplete type definitions is pragmatic.The type annotation on line 265 and the cast on lines 271-272 to set
trailing_commaare necessary because tomlkit'sInlineTabletype doesn't expose this attribute in its type definition. This is a pragmatic workaround for incomplete library types.crate_tools/bump_version.py (6)
24-35: LGTM! TYPE_CHECKING guard follows best practices.The imports are correctly structured with
from __future__ import annotationsat the top, followed bytyping as typandcollections.abc as cabcaliases, and aTYPE_CHECKINGguard forToken. This matches the coding guidelines perfectly.Based on coding guidelines.
91-94: Defensive runtime check improves robustness.Adding a runtime guard to raise
ValueErrorwhentok.mapisNoneprevents potential crashes downstream. This is good defensive coding practice.
139-140: Skip logic prevents crashes on malformed tokens.The check
if not _is_matching_fence_token(tok, lang) or tok.map is None: continuecorrectly skips tokens that don't match the language or lack mapping data, preventing potential crashes.
353-353: Using Path.replace aligns with pathlib guidelines.Replacing
os.replacewithPath(temp_name).replace(toml_path)follows the PTH (pathlib) coding guideline and is more idiomatic modern Python.Based on coding guidelines.
163-172: Type annotations and runtime checks are correctly implemented.The function signature updates with
cabc.MutableMapping[str, typ.Any]and the runtimeisinstancechecks forworkspaceandpackagebeing mappings before mutation are correct. This follows the pattern established in the coding guidelines and maintains type safety.
216-223: Type cast pattern for tomlkit compatibility is pragmatic.The pattern of checking
isinstance(existing, toml_items.String)and then using a cast to access thevaluesetter (with fallback for tomlkit < 0.14) is a practical solution for handling tomlkit's evolving API while maintaining type safety.crate_tools/unittests/test_bump_version.py (4)
1-13: LGTM! Test organization and imports follow guidelines.The module docstring, typing imports, and test organization follow pytest best practices and coding guidelines.
Based on coding guidelines.
21-40: Improved parameter naming and test documentation.The parameter tuple structure and added docstring make the test intent clearer. The parameterization covers both string and dict-style dependencies with version prefix preservation.
80-126: Enhanced test parameterization with semantic outcome parameter.Using
pytest.paramwith explicitid=values and the semanticoutcomeparameter (Literal["update", "preserve"]) significantly improves test readability and output. The logicshould_change = outcome == "update"is clear and maintainable.
71-76: Consider adding test for markdown update failure warning.The previous review comment suggested adding a test that verifies the warning is emitted to stderr when markdown update fails. This test validates the success case but doesn't verify the error handling behavior introduced by
_warn_on_markdown_update_failurein bump_version.py.Consider adding a test that triggers and validates the warning behavior. Example:
def test_markdown_update_failure_emits_warning(tmp_path: Path, capsys: pytest.CaptureFixture[str]) -> None: """Emit warning to stderr when markdown update fails.""" # Create a markdown file with invalid TOML that will fail parsing md_path = tmp_path / "broken.md" md_path.write_text("```toml\n[invalid\n```\n") from crate_tools.bump_version import _warn_on_markdown_update_failure _warn_on_markdown_update_failure(md_path, "1.0.0") captured = capsys.readouterr() assert "Warning: Failed to update" in captured.err assert str(md_path) in captured.errThis would verify the error handling path added in lines 598-607 of bump_version.py.
Use logging-backed warnings, reduce repeated typing annotations with mapping aliases, and cover markdown failure handling with targeted tests.
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: 0
🧹 Nitpick comments (3)
crate_tools/bump_version.py (3)
169-177: Consider extracting nested conditionals into a helper for clarity.The nested
isinstancechecks forworkspaceandpackagecould be extracted into a small helper function to reduce cognitive load and improve testability.Apply this refactor:
+def _get_package_from_workspace(doc: TomlMutableMapping) -> cabc.MutableMapping[str, typ.Any] | None: + """Return the package mapping from workspace if present.""" + workspace = doc.get("workspace") + if isinstance(workspace, cabc.MutableMapping): + package = workspace.get("package") + if isinstance(package, cabc.MutableMapping): + return package + return None + def _update_package_version( doc: TomlMutableMapping, version: str, ) -> None: - workspace = doc.get("workspace") - if isinstance(workspace, cabc.MutableMapping): - package = workspace.get("package") - if isinstance(package, cabc.MutableMapping): - package["version"] = version - return + package = _get_package_from_workspace(doc) + if package is not None: + package["version"] = version + return package = doc.get("package") if isinstance(package, cabc.MutableMapping): package["version"] = version
222-229: Fallback for tomlkit < 0.14 uses private attribute.Accessing
existing._original(line 227) andentry._original(line 255) relies on tomlkit internals. This is a known compatibility shim but increases maintenance burden if tomlkit changes its internals.Consider one of the following:
- Document the minimum supported tomlkit version (0.14+) and remove the fallback.
- Add a test to verify the fallback path works with the oldest supported tomlkit version.
- Wrap the private attribute access in a helper with clear documentation:
def _set_string_value(item: toml_items.String, value: str) -> None: """Set value on a tomlkit String item, handling version differences.""" try: cast_item = typ.cast("typ.Any", item) cast_item.value = value except AttributeError: # tomlkit <0.14 lacks value setter item._original = value # pyright: ignore[reportPrivateUsage]
609-614: Consider usinglogger.exceptionfor automatic traceback.Using
logger.warningwith manual exception formatting works but loses the traceback. If debugging these failures is important, considerlogger.exceptionor at leastexc_info=True:- logger.warning( - "Failed to update Markdown fence versions in %s to %s: %s", - md_path, - version, - exc, - ) + logger.warning( + "Failed to update Markdown fence versions in %s to %s", + md_path, + version, + exc_info=True, + )This provides more diagnostic context without changing the log level.
📜 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)
crate_tools/bump_version.py(17 hunks)crate_tools/unittests/test_bump_version.py(9 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:
crate_tools/unittests/test_bump_version.pycrate_tools/bump_version.py
**/unittests/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Colocate unit tests with code in an unittests/ subdirectory using test_ prefix
Files:
crate_tools/unittests/test_bump_version.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:
crate_tools/unittests/test_bump_version.py
🧬 Code graph analysis (1)
crate_tools/unittests/test_bump_version.py (1)
crate_tools/bump_version.py (4)
_update_dependency_version(284-320)_update_markdown_versions(576-601)_warn_on_markdown_update_failure(604-614)replace_version_in_toml(540-570)
⏰ 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 (17)
crate_tools/bump_version.py (7)
21-41: LGTM! Type imports and aliases follow guidelines.The use of
typing.TYPE_CHECKINGto guard runtime-unnecessary imports,collections.abcaliasing ascabc, and type aliases (TomlMapping,TomlMutableMapping) align with the coding guidelines for type safety and PEP 585/604 conventions.
97-99: LGTM! Runtime guard improves robustness.Adding an explicit check for
tok.map is Nonewith a clear error message prevents obscure failures downstream when fence tokens lack positional data.
145-145: LGTM! Dual guard aligns with token structure expectations.Checking both
_is_matching_fence_tokenandtok.map is Nonebefore processing ensures that only valid, fully populated fence tokens are transformed.
359-359: LGTM! Pathlib atomic replace is safer and more idiomatic.Replacing
os.replace(temp_name, toml_path)withPath(temp_name).replace(toml_path)is more Pythonic and type-safe while maintaining the same atomic semantics.
665-665: LGTM! Centralized error handling improves maintainability.Using the extracted
_warn_on_markdown_update_failurehelper makes the main function cleaner and the error handling testable in isolation.
604-615: Allow ValueError to propagate
ValueError from _process_fence_token signals a programming bug (tokens without map are filtered out), so it should not be caught here.
195-199: Cast safety verified: tomlkit’s Document, Table, and InlineTable implement collections.abc.Mapping, so typ.cast("TomlMapping", entry) is safe; existing fallback covers tomlkit < 0.14.crate_tools/unittests/test_bump_version.py (10)
1-14: LGTM! Imports support new test coverage.Adding
TOMLKitErrorand_warn_on_markdown_update_failureenables testing the error handling path introduced in the main file.
22-31: LGTM! Structured parametrize improves clarity.Changing from
"body, expected, extra"to("body", "expected", "extra")follows pytest best practices for tuple-based parameter specifications.
35-35: LGTM! Descriptive docstring improves test documentation.Adding docstrings to test functions aligns with the coding guideline to document public functions and improves test readability.
44-78: LGTM! Comprehensive edge case coverage.The new tests for trailing comments (line 44), quote style (line 52), missing dependencies (line 62), and workspace dependencies (line 72) provide excellent coverage of formatting preservation and conditional logic.
83-100: LGTM! pytest.param with id improves test output.Using
pytest.param(..., id="toml-fence")makes test failures easier to identify in CI output compared to numeric indices.
135-155: LGTM! Test validates warning behavior.This test addresses the past review comment requesting coverage of markdown update failure warnings. The monkeypatch approach cleanly isolates the error handling logic.
140-141: Pragma comment placement is correct.The
# pragma: no cover - helpercomment is appropriate here since this is a test helper function that only exists to raise an exception for testing purposes.
157-174: LGTM! Validates silent success path.Testing that no warnings are emitted on success is important for preventing log spam in normal operation.
239-257: LGTM! Indentation preservation is critical for nested fences.This test ensures that the fence replacement logic correctly handles indented code blocks, which is important for documentation with nested examples.
176-201: Assertion indices verified. Updated-3/-2line assertions correctly preserve comments; test passes.
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?
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 607 🏁 Script executed: Length of output: 1430 @leynos: I've checked the current state of the code. Here's the status of each suggestion: ✅ Type alias for repeated annotations: RESOLVED
✅ Using logging module in
❌ Simplifying
Two out of three suggestions have been addressed. Would you like me to provide a code example for simplifying the test parameterization? |
|
@coderabbitai Please provide revised guidance and diff for utilizing structural pattern matching here: Structural pattern matching will usually always work better than multiple nested |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 632 🏁 Script executed: Length of output: 717 🏁 Script executed: Length of output: 872 @leynos: Great suggestion! Structural pattern matching is indeed more elegant here. Since the project requires Python >=3.10, we can leverage Here's the revised guidance using structural pattern matching: Approach 1: Inline structural pattern matchingdef _update_package_version(
doc: TomlMutableMapping,
version: str,
) -> None:
"""Update package version in ``doc`` if present.
Examples
--------
>>> data = {"package": {"version": "0"}}
>>> _update_package_version(data, "1")
>>> data["package"]["version"]
'1'
"""
match doc.get("workspace"), doc.get("package"):
case (workspace, _) if isinstance(workspace, cabc.MutableMapping):
package = workspace.get("package")
if isinstance(package, cabc.MutableMapping):
package["version"] = version
return
case (_, package) if isinstance(package, cabc.MutableMapping):
package["version"] = versionApproach 2: Helper function with structural pattern matching (Recommended)This approach improves testability and readability: def _get_package_from_workspace(doc: TomlMutableMapping) -> cabc.MutableMapping[str, typ.Any] | None:
"""Return the package mapping from workspace if present."""
match doc.get("workspace"):
case workspace if isinstance(workspace, cabc.MutableMapping):
match workspace.get("package"):
case package if isinstance(package, cabc.MutableMapping):
return package
return None
def _update_package_version(
doc: TomlMutableMapping,
version: str,
) -> None:
"""Update package version in ``doc`` if present.
Examples
--------
>>> data = {"package": {"version": "0"}}
>>> _update_package_version(data, "1")
>>> data["package"]["version"]
'1'
"""
match _get_package_from_workspace(doc):
case package if package is not None:
package["version"] = version
return
match doc.get("package"):
case package if isinstance(package, cabc.MutableMapping):
package["version"] = versionBenefits of Approach 2:
Both approaches eliminate the nested 🐰 |
|
@coderabbitai How about: Learnings:
Lastly, we are using Python 3.13+ Please revise the guidance with this in mind. |
|
Tip For best results, initiate chat on the files or code changes. @leynos: Excellent! Your approach is much cleaner and more Pythonic. Here's the revised guidance: Revised: Use structural pattern matching with dict deconstructionYour solution elegantly handles both cases without nested conditionals: def _update_package_version(
doc: TomlMutableMapping,
version: str,
) -> None:
"""Update package version in ``doc`` if present.
Examples
--------
>>> data = {"package": {"version": "0"}}
>>> _update_package_version(data, "1")
>>> data["package"]["version"]
'1'
"""
match doc:
case {"workspace": {"package": cabc.MutableMapping() as package}}:
package["version"] = version
case {"package": cabc.MutableMapping() as package}:
package["version"] = versionBenefits:
This is the optimal solution for this use case! 🎯 ✏️ Learnings added
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 408 🏁 Script executed: Length of output: 679 @leynos: Yes, RESOLVED ✅ The structural pattern matching has been implemented exactly as recommended. The current code (lines 169-173) uses: match doc:
case {"workspace": {"package": cabc.MutableMapping() as package}}:
package["version"] = version
case {"package": cabc.MutableMapping() as package}:
package["version"] = versionThis eliminates the nested |
|
@coderabbitai Has the following now been resolved? ❌ Simplifying
|
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 1150 🏁 Script executed: Length of output: 312 🏁 Script executed: Length of output: 687 @leynos: Yes, RESOLVED ✅ The test parameterization has been simplified as requested. Here's what changed: Current implementation (lines 80-129):
What was removed:
The expected content is now parameterized directly, making the test logic cleaner and easier to understand. Each test case explicitly shows both the input markdown and the expected output after version update. 🎯 |
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68dddbb2b824832291d6bd9d6fdb3cdd
Summary by Sourcery
Refactor version bump and publish scripts and associated tests to resolve lint errors by standardizing imports and type hints, adopting modern syntax, normalizing docstrings, and cleaning up test definitions.
Enhancements:
Tests:
Summary by CodeRabbit