Conversation
Reviewer's GuideThe PR refactors mermaid rendering in cli.py by extracting file-writing and CLI-invocation into dedicated helpers, streamlining render_block to orchestrate these helpers, and adds unit tests for both new helpers. 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}
There was a problem hiding this comment.
Hey @leynos - 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> `nixie/cli.py:167` </location>
<code_context>
+
+ success, stderr = await wait_for_proc(proc, path, idx, timeout)
+ if not success:
+ raise RuntimeError(format_cli_error(stderr.decode("utf-8", errors="replace")))
+
+
</code_context>
<issue_to_address>
Consider including more context in the RuntimeError message.
Adding the command or file name to the error message will help identify issues when processing multiple diagrams concurrently.
</issue_to_address>
<suggested_fix>
<<<<<<< SEARCH
if not success:
raise RuntimeError(format_cli_error(stderr.decode("utf-8", errors="replace")))
=======
if not success:
error_message = (
f"Error running command {cmd} for file '{path}' (index {idx}):\n"
f"{format_cli_error(stderr.decode('utf-8', errors='replace'))}"
)
raise RuntimeError(error_message)
>>>>>>> REPLACE
</suggested_fix>
### Comment 2
<location> `nixie/cli.py:212` </location>
<code_context>
+ mmd, svg = _write_diagram_file(block, tmpdir, path, idx)
+ await _run_mermaid_cli(mmd, svg, cfg_path, semaphore, path, idx, timeout)
+ return True
+ except FileNotFoundError as exc:
+ cli = exc.filename or "mmdc"
+ print(
+ (
+ f"Error: '{cli}' not found. Install Node.js with npx "
+ "or Bun to use @mermaid-js/mermaid-cli."
+ ),
+ file=sys.stderr,
+ )
+ except RuntimeError as exc:
+ print(exc, file=sys.stderr)
</code_context>
<issue_to_address>
The fallback to 'mmdc' as the CLI name may be misleading if a different executable is missing.
If get_mmdc_cmd returns a different executable, the error message may incorrectly reference 'mmdc'. Use the actual command attempted for clarity.
Suggested implementation:
```python
mmd, svg = _write_diagram_file(block, tmpdir, path, idx)
# Determine the actual command used for mermaid CLI
mmdc_cmd = get_mmdc_cmd() # This should return the command as a list or string
await _run_mermaid_cli(mmd, svg, cfg_path, semaphore, path, idx, timeout, mmdc_cmd=mmdc_cmd)
return True
except FileNotFoundError as exc:
# Use the actual command attempted, fallback to exc.filename if available, else use mmdc_cmd[0] or "mmdc"
if hasattr(exc, "filename") and exc.filename:
cli = exc.filename
elif isinstance(mmdc_cmd, (list, tuple)) and mmdc_cmd:
cli = mmdc_cmd[0]
elif isinstance(mmdc_cmd, str):
cli = mmdc_cmd
else:
cli = "mmdc"
print(
(
f"Error: '{cli}' not found. Install Node.js with npx "
"or Bun to use @mermaid-js/mermaid-cli."
),
file=sys.stderr,
)
```
- Ensure that `get_mmdc_cmd()` is defined and returns the actual command used to invoke the mermaid CLI.
- If `_run_mermaid_cli` already determines the command internally, you may need to refactor it to return or expose the command so it can be referenced in the except block.
- If `get_mmdc_cmd()` requires arguments, adjust the call accordingly.
- Pass `mmdc_cmd` to `_run_mermaid_cli` if it needs to use the same command.
</issue_to_address>
### Comment 3
<location> `nixie/cli.py:109` </location>
<code_context>
return success, stderr
+def _write_diagram_file(
+ block: str, tmpdir: Path, path: Path, idx: int
+) -> tuple[Path, Path]:
</code_context>
<issue_to_address>
Consider merging the file-writing and CLI-invocation helpers into a single function to reduce indirection and simplify parameter passing.
```markdown
Consider merging `_write_diagram_file` and `_run_mermaid_cli` into a single helper to reduce indirection and long parameter lists. For example:
```python
async def _render_diagram(
block: str,
tmpdir: Path,
cfg_path: Path,
path: Path,
idx: int,
sem: asyncio.Semaphore,
timeout: float,
) -> None:
# inline file-write
mmd = tmpdir / f"{path.stem}_{idx}.mmd"
svg = mmd.with_suffix(".svg")
mmd.write_text(block)
# all CLI logic here
cmd = get_mmdc_cmd(mmd, svg, cfg_path)
logging.getLogger(__name__).info(shlex.join(cmd))
async with sem:
proc = await asyncio.create_subprocess_exec(
*cmd,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE,
)
success, stderr = await wait_for_proc(proc, path, idx, timeout)
if not success:
raise RuntimeError(format_cli_error(stderr.decode("utf-8", errors="replace")))
```
Then simplify `render_block` to:
```python
async def render_block(
block: str,
tmpdir: Path,
cfg_path: Path,
path: Path,
idx: int,
semaphore: asyncio.Semaphore,
timeout: float = 30.0,
) -> bool:
try:
await _render_diagram(block, tmpdir, cfg_path, path, idx, semaphore, timeout)
return True
except FileNotFoundError as exc:
cli = exc.filename or "mmdc"
print(f"Error: '{cli}' not found. Install Node.js with npx or Bun.", file=sys.stderr)
except RuntimeError as exc:
print(exc, file=sys.stderr)
except Exception as exc:
print(f"{path}: unexpected error in diagram {idx}", file=sys.stderr)
traceback.print_exception(type(exc), exc, exc.__traceback__, file=sys.stderr)
return False
```
This removes a trivial file‐write helper, consolidates CLI + semaphore logic into one place, and keeps your API of `render_block(...)` unchanged.
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return success, stderr | ||
|
|
||
|
|
||
| def _write_diagram_file( |
There was a problem hiding this comment.
issue (complexity): Consider merging the file-writing and CLI-invocation helpers into a single function to reduce indirection and simplify parameter passing.
Consider merging `_write_diagram_file` and `_run_mermaid_cli` into a single helper to reduce indirection and long parameter lists. For example:
```python
async def _render_diagram(
block: str,
tmpdir: Path,
cfg_path: Path,
path: Path,
idx: int,
sem: asyncio.Semaphore,
timeout: float,
) -> None:
# inline file-write
mmd = tmpdir / f"{path.stem}_{idx}.mmd"
svg = mmd.with_suffix(".svg")
mmd.write_text(block)
# all CLI logic here
cmd = get_mmdc_cmd(mmd, svg, cfg_path)
logging.getLogger(__name__).info(shlex.join(cmd))
async with sem:
proc = await asyncio.create_subprocess_exec(
*cmd,
stdout=asyncio.subprocess.PIPE,
stderr=asyncio.subprocess.PIPE,
)
success, stderr = await wait_for_proc(proc, path, idx, timeout)
if not success:
raise RuntimeError(format_cli_error(stderr.decode("utf-8", errors="replace")))Then simplify render_block to:
async def render_block(
block: str,
tmpdir: Path,
cfg_path: Path,
path: Path,
idx: int,
semaphore: asyncio.Semaphore,
timeout: float = 30.0,
) -> bool:
try:
await _render_diagram(block, tmpdir, cfg_path, path, idx, semaphore, timeout)
return True
except FileNotFoundError as exc:
cli = exc.filename or "mmdc"
print(f"Error: '{cli}' not found. Install Node.js with npx or Bun.", file=sys.stderr)
except RuntimeError as exc:
print(exc, file=sys.stderr)
except Exception as exc:
print(f"{path}: unexpected error in diagram {idx}", file=sys.stderr)
traceback.print_exception(type(exc), exc, exc.__traceback__, file=sys.stderr)
return FalseThis removes a trivial file‐write helper, consolidates CLI + semaphore logic into one place, and keeps your API of render_block(...) unchanged.
There was a problem hiding this comment.
Has this now been resolved?
There was a problem hiding this comment.
The issue has not been resolved yet. The helpers _write_diagram_file and _run_mermaid_cli are still implemented as separate functions, and render_block continues to coordinate between them. The suggestion to merge these helpers into a single function for reduced indirection and simpler parameter passing has not been incorporated in the current state of the code.
If you would like to discuss the trade-offs or revisit the refactor suggestion, let me know!
There was a problem hiding this comment.
Has this now been resolved?
There was a problem hiding this comment.
No, this has not been resolved yet. The helpers _write_diagram_file and _run_mermaid_cli are still implemented as separate functions in nixie/cli.py, and have not been merged into a single function as previously suggested. If you'd like to revisit this refactor or discuss the pros and cons, let me know—I'm happy to help!
There was a problem hiding this comment.
Has this now been resolved?
There was a problem hiding this comment.
No, this has not been resolved yet. The functions _write_diagram_file and _run_mermaid_cli are still implemented as separate helpers in nixie/cli.py, and have not been merged into a single function as previously suggested. If you'd like to proceed with this refactor or discuss the pros and cons, let me know—I'm here to 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. Summary by CodeRabbit
WalkthroughRefactor the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant render_block
participant _write_diagram_file
participant _run_mermaid_cli
participant MermaidCLI
User->>render_block: Request to render diagram block
render_block->>_write_diagram_file: Write Mermaid block to .mmd file
_write_diagram_file-->>render_block: Return .mmd and .svg paths
render_block->>_run_mermaid_cli: Invoke Mermaid CLI asynchronously
_run_mermaid_cli->>MermaidCLI: Run mmdc to convert .mmd to .svg
MermaidCLI-->>_run_mermaid_cli: Return success or error
_run_mermaid_cli-->>render_block: Raise exception or return
render_block-->>User: Return True (success) or False (failure)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~15 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![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)
nixie/cli.py (1)
136-167: Robust async helper with proper error handling.The function correctly uses the semaphore for concurrency control and raises appropriate exceptions. The nosemgrep comment is justified for this controlled subprocess usage.
Note: Past review comments suggested including more context in the RuntimeError message and merging this helper with
_write_diagram_file. The current separation provides good testability, though merging remains a valid architectural choice.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (3)
nixie/cli.py(2 hunks)nixie/unittests/test_run_mermaid_cli.py(1 hunks)nixie/unittests/test_write_diagram_file.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit Inference Engine (AGENTS.md)
For Python files: Pass all relevant unit and behavioral tests, pass lint checks (
ruff check), adhere to formatting standards (ruff format), and pass type checking (pyright).
**/*.py: Use snake_case.py for file names, naming files for their contents (e.g., http_client.py, task_queue.py)
Use PascalCase for class names in Python files
Use snake_case for variable and function names in Python files
Use UPPER_SNAKE_CASE for module-level constants in Python files
Prefix private/internal helpers or APIs with a single underscore (_) in Python files
Use typing everywhere and maintain full static type coverage in Python files; use Pyright for type-checking
Use TypedDict or Dataclass for structured data where appropriate; for internal-only usage, prefer @DataClass(slots=True)
Avoid Any in type annotations; use Unknown, generics, or cast() when necessary, and always document why Any is acceptable if used
Be explicit with return types in public functions and class methods (e.g., use -> None, -> str, etc.)
Favor immutability in Python files; prefer tuples over lists, and frozendict or types.MappingProxyType where appropriate
Enable Ruff for linting and formatting; use Ruff to lint for performance, security, consistency, and style issues, and let Ruff handle whitespace and formatting entirely
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly and with explanation
Avoid side effects at import time in Python modules; modules should not modify global state or perform actions on import
Use docstrings to document public functions, classes, and modules using NumPy format
Explain tricky code with inline comments for non-obvious logic or decisions in Python files
**/*.py: Use context managers to encapsulate setup and teardown logic cleanly and safely, especially for resource management (files, locks, connections, etc.), instead of manual try/finally blocks.
Use@contextmanagerfromcontextlibfor straightforward proce...
Files:
nixie/unittests/test_run_mermaid_cli.pynixie/unittests/test_write_diagram_file.pynixie/cli.py
⚙️ CodeRabbit Configuration File
**/*.py: How about the following?- Keep cyclomatic complexity ≤ 12 - Follow single responsibility and CQRS (command/query segregation) - Docstrings must follow the `numpy` style guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces. - Move conditionals with >2 branches to predicate/helper functions - Avoid `eval`, `exec`, `pickle`, monkey-patching, `ctypes`, unsafe shell - Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate. - Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar - Lint suppressions: - Blanket `# noqa`, file-level skips, and categories are **forbidden** - Only narrow in-line disables (`# noqa: XYZ`) are permitted, and must be accompanied by `FIXME:` or a ticket link, and used only as a last resort. - Use `pytest` fixtures for shared setup (`conftest.py` or `fixtures/`) - Replace duplicate tests with `@pytest.mark.parametrize` - Prefer `pytest-mock` or `unittest.mock` for stubs/mocks - Use `assert …, "message"` over bare asserts - Reflect all API/behaviour changes in `docs/` and update roadmap on completion - Files must not exceed 400 logical lines: - Decompose large modules into subpackages - Split large `match/case` or dispatch tables by domain and collocate with targets if appropriate - Move bulky data (fixtures, templates) to external files for parsing at runtime - Mutable defaults and shadowed built-ins are forbidden - All code must have clear type hints using modern style (`A | B`, `list[str]`, `class Foo[A]:`, `type Bar = int`, etc.), with ABC imports drawn from the correct stdlib module.
Files:
nixie/unittests/test_run_mermaid_cli.pynixie/unittests/test_write_diagram_file.pynixie/cli.py
**/unittests/test_*.py
📄 CodeRabbit Inference Engine (.rules/python-00.mdc)
Colocate unit tests with code using a unittests subdirectory and a test_ prefix for test files
Files:
nixie/unittests/test_run_mermaid_cli.pynixie/unittests/test_write_diagram_file.py
**/test_*.py
📄 CodeRabbit Inference Engine (.rules/python-00.mdc)
**/test_*.py: Use pytest idioms in test files: prefer fixtures over setup/teardown methods, parametrize broadly, and avoid unnecessary mocks
Group related tests using class with method names prefixed by test_ in test files
Files:
nixie/unittests/test_run_mermaid_cli.pynixie/unittests/test_write_diagram_file.py
🧬 Code Graph Analysis (2)
nixie/unittests/test_run_mermaid_cli.py (1)
nixie/cli.py (3)
cli(314-321)_run_mermaid_cli(136-167)get_mmdc_cmd(61-78)
nixie/unittests/test_write_diagram_file.py (1)
nixie/cli.py (2)
cli(314-321)_write_diagram_file(109-133)
🪛 Ruff (0.12.2)
nixie/unittests/test_run_mermaid_cli.py
28-28: Unused function argument: cmd
(ARG001)
28-28: Unused function argument: kwargs
(ARG001)
32-32: Unused function argument: proc
(ARG001)
32-32: Unused function argument: path
(ARG001)
32-32: Unused function argument: idx
(ARG001)
32-32: Async function definition with a timeout parameter
(ASYNC109)
32-32: Unused function argument: timeout
(ARG001)
32-32: Trailing comma missing
Add trailing comma
(COM812)
49-49: Trailing comma missing
Add trailing comma
(COM812)
59-59: Unused function argument: cmd
(ARG001)
59-59: Unused function argument: kwargs
(ARG001)
63-63: Unused function argument: proc
(ARG001)
63-63: Unused function argument: path
(ARG001)
63-63: Unused function argument: idx
(ARG001)
63-63: Async function definition with a timeout parameter
(ASYNC109)
63-63: Unused function argument: timeout
(ARG001)
63-63: Trailing comma missing
Add trailing comma
(COM812)
nixie/cli.py
110-110: Trailing comma missing
Add trailing comma
(COM812)
143-143: Async function definition with a timeout parameter
(ASYNC109)
211-211: Consider moving this statement to an else block
(TRY300)
223-223: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (5)
nixie/unittests/test_write_diagram_file.py (1)
8-16: LGTM! Well-structured test coverage.The test effectively verifies the core functionality of
_write_diagram_file: writing the mermaid source to a.mmdfile, generating the correct SVG path, and confirming file creation.nixie/unittests/test_run_mermaid_cli.py (2)
14-44: Excellent test coverage for the success path.The test properly mocks the subprocess execution and verifies that the command is logged correctly. The monkeypatching approach is appropriate for testing async subprocess operations.
47-74: Solid error handling test.The test correctly verifies that
_run_mermaid_cliraises aRuntimeErrorwith the expected error message when the subprocess fails.nixie/cli.py (2)
109-133: Well-structured helper function with comprehensive documentation.The function follows the coding guidelines with proper numpy-style docstrings and clear parameter descriptions. The implementation is straightforward and testable.
Fix the trailing comma issue:
def _write_diagram_file( - block: str, tmpdir: Path, path: Path, idx: int + block: str, tmpdir: Path, path: Path, idx: int, ) -> tuple[Path, Path]:
208-226: Refactor improves error handling and modularity.The extracted helpers make the code more testable and the error handling clearer.
Improve the try/except structure by moving the success return to an else block:
try: mmd, svg = _write_diagram_file(block, tmpdir, path, idx) await _run_mermaid_cli(mmd, svg, cfg_path, semaphore, path, idx, timeout) - return True except FileNotFoundError as exc: cli = exc.filename or "mmdc" print( ( f"Error: '{cli}' not found. Install Node.js with npx " "or Bun to use @mermaid-js/mermaid-cli." ), file=sys.stderr, ) except RuntimeError as exc: print(exc, file=sys.stderr) except Exception as exc: print(f"{path}: unexpected error in diagram {idx}", file=sys.stderr) traceback.print_exception(type(exc), exc, exc.__traceback__, file=sys.stderr) +else: + return True return False
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
nixie/cli.py(2 hunks)nixie/unittests/test_run_mermaid_cli.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit Inference Engine (AGENTS.md)
For Python files: Pass all relevant unit and behavioral tests, pass lint checks (
ruff check), adhere to formatting standards (ruff format), and pass type checking (pyright).
**/*.py: Use snake_case.py for file names, naming files for their contents (e.g., http_client.py, task_queue.py)
Use PascalCase for class names in Python files
Use snake_case for variable and function names in Python files
Use UPPER_SNAKE_CASE for module-level constants in Python files
Prefix private/internal helpers or APIs with a single underscore (_) in Python files
Use typing everywhere and maintain full static type coverage in Python files; use Pyright for type-checking
Use TypedDict or Dataclass for structured data where appropriate; for internal-only usage, prefer @DataClass(slots=True)
Avoid Any in type annotations; use Unknown, generics, or cast() when necessary, and always document why Any is acceptable if used
Be explicit with return types in public functions and class methods (e.g., use -> None, -> str, etc.)
Favor immutability in Python files; prefer tuples over lists, and frozendict or types.MappingProxyType where appropriate
Enable Ruff for linting and formatting; use Ruff to lint for performance, security, consistency, and style issues, and let Ruff handle whitespace and formatting entirely
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly and with explanation
Avoid side effects at import time in Python modules; modules should not modify global state or perform actions on import
Use docstrings to document public functions, classes, and modules using NumPy format
Explain tricky code with inline comments for non-obvious logic or decisions in Python files
**/*.py: Use context managers to encapsulate setup and teardown logic cleanly and safely, especially for resource management (files, locks, connections, etc.), instead of manual try/finally blocks.
Use@contextmanagerfromcontextlibfor straightforward proce...
Files:
nixie/unittests/test_run_mermaid_cli.pynixie/cli.py
⚙️ CodeRabbit Configuration File
**/*.py: How about the following?- Keep cyclomatic complexity ≤ 12 - Follow single responsibility and CQRS (command/query segregation) - Docstrings must follow the `numpy` style guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces. - Move conditionals with >2 branches to predicate/helper functions - Avoid `eval`, `exec`, `pickle`, monkey-patching, `ctypes`, unsafe shell - Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate. - Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar - Lint suppressions: - Blanket `# noqa`, file-level skips, and categories are **forbidden** - Only narrow in-line disables (`# noqa: XYZ`) are permitted, and must be accompanied by `FIXME:` or a ticket link, and used only as a last resort. - Use `pytest` fixtures for shared setup (`conftest.py` or `fixtures/`) - Replace duplicate tests with `@pytest.mark.parametrize` - Prefer `pytest-mock` or `unittest.mock` for stubs/mocks - Use `assert …, "message"` over bare asserts - Reflect all API/behaviour changes in `docs/` and update roadmap on completion - Files must not exceed 400 logical lines: - Decompose large modules into subpackages - Split large `match/case` or dispatch tables by domain and collocate with targets if appropriate - Move bulky data (fixtures, templates) to external files for parsing at runtime - Mutable defaults and shadowed built-ins are forbidden - All code must have clear type hints using modern style (`A | B`, `list[str]`, `class Foo[A]:`, `type Bar = int`, etc.), with ABC imports drawn from the correct stdlib module.
Files:
nixie/unittests/test_run_mermaid_cli.pynixie/cli.py
**/unittests/test_*.py
📄 CodeRabbit Inference Engine (.rules/python-00.mdc)
Colocate unit tests with code using a unittests subdirectory and a test_ prefix for test files
Files:
nixie/unittests/test_run_mermaid_cli.py
**/test_*.py
📄 CodeRabbit Inference Engine (.rules/python-00.mdc)
**/test_*.py: Use pytest idioms in test files: prefer fixtures over setup/teardown methods, parametrize broadly, and avoid unnecessary mocks
Group related tests using class with method names prefixed by test_ in test files
Files:
nixie/unittests/test_run_mermaid_cli.py
🧠 Learnings (1)
📚 Learning: in the nixie codebase, trailing commas are only used when function parameters are formatted one per ...
Learnt from: leynos
PR: leynos/nixie#10
File: nixie/unittests/test_run_mermaid_cli.py:32-34
Timestamp: 2025-08-07T09:30:31.098Z
Learning: In the nixie codebase, trailing commas are only used when function parameters are formatted one per line (each parameter on its own line), not when parameters are simply wrapped to multiple lines but grouped together. Example of when trailing comma is acceptable: `def foo(a,\n b,\n c,\n) -> d:` vs when it's not needed: `def foo(\n a, b, c\n) -> d:`
Applied to files:
nixie/unittests/test_run_mermaid_cli.py
🧬 Code Graph Analysis (1)
nixie/unittests/test_run_mermaid_cli.py (1)
nixie/cli.py (3)
cli(318-325)_run_mermaid_cli(136-169)get_mmdc_cmd(61-78)
🪛 Ruff (0.12.2)
nixie/unittests/test_run_mermaid_cli.py
29-29: Unused function argument: cmd
(ARG001)
29-29: Unused function argument: kwargs
(ARG001)
33-33: Unused function argument: proc
(ARG001)
33-33: Unused function argument: path
(ARG001)
33-33: Unused function argument: idx
(ARG001)
33-33: Async function definition with a timeout parameter
(ASYNC109)
33-33: Unused function argument: timeout
(ARG001)
33-33: Trailing comma missing
Add trailing comma
(COM812)
50-50: Trailing comma missing
Add trailing comma
(COM812)
61-61: Unused function argument: cmd
(ARG001)
61-61: Unused function argument: kwargs
(ARG001)
65-65: Unused function argument: proc
(ARG001)
65-65: Unused function argument: path
(ARG001)
65-65: Unused function argument: idx
(ARG001)
65-65: Async function definition with a timeout parameter
(ASYNC109)
65-65: Unused function argument: timeout
(ARG001)
65-65: Trailing comma missing
Add trailing comma
(COM812)
nixie/cli.py
110-110: Trailing comma missing
Add trailing comma
(COM812)
141-141: Async function definition with a timeout parameter
(ASYNC109)
215-215: Consider moving this statement to an else block
(TRY300)
227-227: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (6)
nixie/unittests/test_run_mermaid_cli.py (3)
14-46: Excellent test coverage for successful CLI invocation.The test effectively verifies that
_run_mermaid_cliproperly logs commands and handles successful execution through well-crafted fakes.
48-79: Solid error handling test implementation.The test thoroughly verifies that
_run_mermaid_cliraisesRuntimeErrorwith appropriate error messages when the CLI fails, including proper context about the file and error details.
29-35: Ignore the unused argument warnings from Ruff.The static analysis tool flags unused arguments in the fake functions, but these parameters are required to match the signatures of the real functions being mocked. This is a false positive.
nixie/cli.py (3)
109-133: Clean and focused file writing helper.The function properly encapsulates the diagram file creation logic with clear documentation following NumPy style. The implementation is straightforward and returns both the input and expected output paths as needed.
136-170: Robust CLI invocation with proper error handling.The function correctly handles subprocess execution within the semaphore, provides comprehensive error messages including command details, and properly decodes stderr with error handling. The docstring follows NumPy format and clearly documents the exceptions raised.
210-230: Well-orchestrated refactor of render_block.The function now serves as a clear coordinator, delegating file writing and CLI execution to the helper functions whilst centralising error handling. The error messages provide appropriate context for different failure scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
nixie/cli.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit Inference Engine (AGENTS.md)
For Python files: Pass all relevant unit and behavioral tests, pass lint checks (
ruff check), adhere to formatting standards (ruff format), and pass type checking (pyright).
**/*.py: Use snake_case.py for file names, naming files for their contents (e.g., http_client.py, task_queue.py)
Use PascalCase for class names in Python files
Use snake_case for variable and function names in Python files
Use UPPER_SNAKE_CASE for module-level constants in Python files
Prefix private/internal helpers or APIs with a single underscore (_) in Python files
Use typing everywhere and maintain full static type coverage in Python files; use Pyright for type-checking
Use TypedDict or Dataclass for structured data where appropriate; for internal-only usage, prefer @DataClass(slots=True)
Avoid Any in type annotations; use Unknown, generics, or cast() when necessary, and always document why Any is acceptable if used
Be explicit with return types in public functions and class methods (e.g., use -> None, -> str, etc.)
Favor immutability in Python files; prefer tuples over lists, and frozendict or types.MappingProxyType where appropriate
Enable Ruff for linting and formatting; use Ruff to lint for performance, security, consistency, and style issues, and let Ruff handle whitespace and formatting entirely
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly and with explanation
Avoid side effects at import time in Python modules; modules should not modify global state or perform actions on import
Use docstrings to document public functions, classes, and modules using NumPy format
Explain tricky code with inline comments for non-obvious logic or decisions in Python files
**/*.py: Use context managers to encapsulate setup and teardown logic cleanly and safely, especially for resource management (files, locks, connections, etc.), instead of manual try/finally blocks.
Use@contextmanagerfromcontextlibfor straightforward proce...
Files:
nixie/cli.py
⚙️ CodeRabbit Configuration File
**/*.py: How about the following?- Keep cyclomatic complexity ≤ 12 - Follow single responsibility and CQRS (command/query segregation) - Docstrings must follow the `numpy` style guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces. - Move conditionals with >2 branches to predicate/helper functions - Avoid `eval`, `exec`, `pickle`, monkey-patching, `ctypes`, unsafe shell - Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate. - Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar - Lint suppressions: - Blanket `# noqa`, file-level skips, and categories are **forbidden** - Only narrow in-line disables (`# noqa: XYZ`) are permitted, and must be accompanied by `FIXME:` or a ticket link, and used only as a last resort. - Use `pytest` fixtures for shared setup (`conftest.py` or `fixtures/`) - Replace duplicate tests with `@pytest.mark.parametrize` - Prefer `pytest-mock` or `unittest.mock` for stubs/mocks - Use `assert …, "message"` over bare asserts - Reflect all API/behaviour changes in `docs/` and update roadmap on completion - Files must not exceed 400 logical lines: - Decompose large modules into subpackages - Split large `match/case` or dispatch tables by domain and collocate with targets if appropriate - Move bulky data (fixtures, templates) to external files for parsing at runtime - Mutable defaults and shadowed built-ins are forbidden - All code must have clear type hints using modern style (`A | B`, `list[str]`, `class Foo[A]:`, `type Bar = int`, etc.), with ABC imports drawn from the correct stdlib module.
Files:
nixie/cli.py
🪛 Ruff (0.12.2)
nixie/cli.py
110-110: Trailing comma missing
Add trailing comma
(COM812)
145-145: Async function definition with a timeout parameter
(ASYNC109)
233-233: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (2)
nixie/cli.py (2)
140-177: Well-structured async helper with comprehensive error handling.The function correctly isolates CLI invocation logic with proper concurrency control and detailed error reporting. The timeout parameter is appropriately used for subprocess management.
217-238: Clean refactoring with proper error handling structure.The refactored function successfully delegates responsibilities to the helper functions whilst maintaining comprehensive error handling. The else clause clearly delineates the success path, and the broad Exception catch is appropriate for this top-level handler that provides debugging information for unexpected errors.
leynos
force-pushed
the
codex/refactor-diagram-file-creation-and-cli-invocation
branch
from
c888239 to
64368b1
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
nixie/cli.py (1)
179-210: Remove or implement the unusedverboseparameter.The
verboseparameter is documented in the docstring but not used in the implementation. Either remove it from the function signature and docstring, or implement the logging control as described.- *, - verbose: bool | None = None, timeout: float = 30.0,And remove the corresponding documentation from the docstring.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
.gitignore(1 hunks)CRUSH.md(1 hunks)Makefile(1 hunks)nixie/cli.py(2 hunks)nixie/unittests/test_run_mermaid_cli.py(1 hunks)nixie/unittests/test_write_diagram_file.py(1 hunks)pyproject.toml(1 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.md
📄 CodeRabbit Inference Engine (AGENTS.md)
For Markdown files (
.mdonly): Pass lint checks (markdownlint filename.md).
Files:
CRUSH.md
⚙️ CodeRabbit Configuration File
**/*.md: * Avoid 2nd person or 1st person pronouns ("I", "you", "we")
- Use en-GB-oxendict (-ize / -our) spelling and grammar
- Paragraphs and bullets must be wrapped to 80 columns, except where a long URL would prevent this (in which case, silence MD013 for that line)
- Code blocks should be wrapped to 120 columns.
- Headings must not be wrapped.
- Documents must start with a level 1 heading
- Headings must correctly increase or decrease by no more than one level at a time
- Use GitHub-flavoured Markdown style for footnotes and endnotes.
- Numbered footnotes must be numbered by order of appearance in the document.
Files:
CRUSH.md
**/*.py
📄 CodeRabbit Inference Engine (AGENTS.md)
For Python files: Pass all relevant unit and behavioral tests, pass lint checks (
ruff check), adhere to formatting standards (ruff format), and pass type checking (pyright).
**/*.py: Use snake_case.py for file names, naming files for their contents (e.g., http_client.py, task_queue.py)
Use PascalCase for class names in Python files
Use snake_case for variable and function names in Python files
Use UPPER_SNAKE_CASE for module-level constants in Python files
Prefix private/internal helpers or APIs with a single underscore (_) in Python files
Use typing everywhere and maintain full static type coverage in Python files; use Pyright for type-checking
Use TypedDict or Dataclass for structured data where appropriate; for internal-only usage, prefer @DataClass(slots=True)
Avoid Any in type annotations; use Unknown, generics, or cast() when necessary, and always document why Any is acceptable if used
Be explicit with return types in public functions and class methods (e.g., use -> None, -> str, etc.)
Favor immutability in Python files; prefer tuples over lists, and frozendict or types.MappingProxyType where appropriate
Enable Ruff for linting and formatting; use Ruff to lint for performance, security, consistency, and style issues, and let Ruff handle whitespace and formatting entirely
Enforce strict mode in Pyright and treat all Pyright warnings as CI errors; use # pyright: ignore sparingly and with explanation
Avoid side effects at import time in Python modules; modules should not modify global state or perform actions on import
Use docstrings to document public functions, classes, and modules using NumPy format
Explain tricky code with inline comments for non-obvious logic or decisions in Python files
**/*.py: Use context managers to encapsulate setup and teardown logic cleanly and safely, especially for resource management (files, locks, connections, etc.), instead of manual try/finally blocks.
Use@contextmanagerfromcontextlibfor straightforward proce...
Files:
nixie/unittests/test_write_diagram_file.pynixie/cli.pynixie/unittests/test_run_mermaid_cli.py
⚙️ CodeRabbit Configuration File
**/*.py: How about the following?- Keep cyclomatic complexity ≤ 12 - Follow single responsibility and CQRS (command/query segregation) - Docstrings must follow the `numpy` style guide. Use a single-line summary for private functions and methods, and full structured docs for all public interfaces. - Move conditionals with >2 branches to predicate/helper functions - Avoid `eval`, `exec`, `pickle`, monkey-patching, `ctypes`, unsafe shell - Every module must begin with a triple-quoted docstring explaining its purpose, utility, and usage, including example calls if appropriate. - Comments and docs must follow en-GB-oxendict (-ize / -our) spelling and grammar - Lint suppressions: - Blanket `# noqa`, file-level skips, and categories are **forbidden** - Only narrow in-line disables (`# noqa: XYZ`) are permitted, and must be accompanied by `FIXME:` or a ticket link, and used only as a last resort. - Use `pytest` fixtures for shared setup (`conftest.py` or `fixtures/`) - Replace duplicate tests with `@pytest.mark.parametrize` - Prefer `pytest-mock` or `unittest.mock` for stubs/mocks - Use `assert …, "message"` over bare asserts - Reflect all API/behaviour changes in `docs/` and update roadmap on completion - Files must not exceed 400 logical lines: - Decompose large modules into subpackages - Split large `match/case` or dispatch tables by domain and collocate with targets if appropriate - Move bulky data (fixtures, templates) to external files for parsing at runtime - Mutable defaults and shadowed built-ins are forbidden - All code must have clear type hints using modern style (`A | B`, `list[str]`, `class Foo[A]:`, `type Bar = int`, etc.), with ABC imports drawn from the correct stdlib module.
Files:
nixie/unittests/test_write_diagram_file.pynixie/cli.pynixie/unittests/test_run_mermaid_cli.py
**/unittests/test_*.py
📄 CodeRabbit Inference Engine (.rules/python-00.mdc)
Colocate unit tests with code using a unittests subdirectory and a test_ prefix for test files
Files:
nixie/unittests/test_write_diagram_file.pynixie/unittests/test_run_mermaid_cli.py
**/test_*.py
📄 CodeRabbit Inference Engine (.rules/python-00.mdc)
**/test_*.py: Use pytest idioms in test files: prefer fixtures over setup/teardown methods, parametrize broadly, and avoid unnecessary mocks
Group related tests using class with method names prefixed by test_ in test files
Files:
nixie/unittests/test_write_diagram_file.pynixie/unittests/test_run_mermaid_cli.py
pyproject.toml
📄 CodeRabbit Inference Engine (.rules/python-00.mdc)
Use pyproject.toml to configure tools like Ruff, Pyright, and Pytest
pyproject.toml: Always usepyproject.tomlas the central configuration file for Astral uv projects; do not userequirements.txtorsetup.py.
The[project]table inpyproject.tomlmust include at leastnameandversionfields as required by PEP 621.
Includedescriptionandreadmefields in the[project]table to improve indexing and packaging tool compatibility.
Specifyrequires-pythonin the[project]table to constrain supported Python versions.
Use thelicensefield in[project]as either an SPDX identifier (license = { text = "MIT" }) or by referencing a file.
List authors in the[project]table as a list of tables withnameand
Includekeywordsandclassifiersin the[project]table to improve discoverability and registry compatibility.
Declare runtime dependencies in the[project].dependencieslist using PEP 508 syntax.
Use[project.optional-dependencies]to define groups such asdevanddocsfor development and documentation dependencies.
Define CLI entry points using[project.scripts]and GUI entry points using[project.gui-scripts]inpyproject.toml.
Register plugin entry points using[project.entry-points.'group.name']inpyproject.tomlif your project supports plugins.
Declare a[build-system]table withrequires(e.g.,setuptools>=61.0,wheel) andbuild-backend(e.g.,setuptools.build_meta) for modern builds and editable installs.
If[build-system]is omitted, set[tool.uv].package = trueto ensure your project is built and installed by uv.
Set[tool.uv].package = trueto force uv to build and install your project into its virtual environment on everyuv syncoruv run.
Keeppyproject.tomlhuman-readable and edit it by hand when possible.
Follow semantic versioning (semver) for theversionfield inpyproject.toml.
Keep build constraints minimal in[build-system]; omit it if you do n...
Files:
pyproject.toml
🧠 Learnings (1)
📚 Learning: 2025-08-07T09:30:31.098Z
Learnt from: leynos
PR: leynos/nixie#10
File: nixie/unittests/test_run_mermaid_cli.py:32-34
Timestamp: 2025-08-07T09:30:31.098Z
Learning: In the nixie codebase, trailing commas are only used when function parameters are formatted one per line (each parameter on its own line), not when parameters are simply wrapped to multiple lines but grouped together. Example of when trailing comma is acceptable: `def foo(a,\n b,\n c,\n) -> d:` vs when it's not needed: `def foo(\n a, b, c\n) -> d:`
Applied to files:
nixie/cli.pynixie/unittests/test_run_mermaid_cli.py
🧬 Code Graph Analysis (2)
nixie/unittests/test_write_diagram_file.py (1)
nixie/cli.py (2)
cli(320-327)_write_diagram_file(109-137)
nixie/unittests/test_run_mermaid_cli.py (1)
nixie/cli.py (3)
cli(320-327)_run_mermaid_cli(140-176)get_mmdc_cmd(61-78)
🪛 Ruff (0.12.2)
nixie/cli.py
110-110: Trailing comma missing
Add trailing comma
(COM812)
145-145: Async function definition with a timeout parameter
(ASYNC109)
227-227: Do not catch blind exception: Exception
(BLE001)
nixie/unittests/test_run_mermaid_cli.py
29-29: Unused function argument: cmd
(ARG001)
29-29: Unused function argument: kwargs
(ARG001)
33-33: Unused function argument: proc
(ARG001)
33-33: Unused function argument: path
(ARG001)
33-33: Unused function argument: idx
(ARG001)
33-33: Async function definition with a timeout parameter
(ASYNC109)
33-33: Unused function argument: timeout
(ARG001)
33-33: Trailing comma missing
Add trailing comma
(COM812)
50-50: Trailing comma missing
Add trailing comma
(COM812)
61-61: Unused function argument: cmd
(ARG001)
61-61: Unused function argument: kwargs
(ARG001)
65-65: Unused function argument: proc
(ARG001)
65-65: Unused function argument: path
(ARG001)
65-65: Unused function argument: idx
(ARG001)
65-65: Async function definition with a timeout parameter
(ASYNC109)
65-65: Unused function argument: timeout
(ARG001)
65-65: Trailing comma missing
Add trailing comma
(COM812)
🔇 Additional comments (9)
.gitignore (1)
194-200: LGTM!The gitignore additions are appropriate and well-documented. The .crush pattern aligns with the new tooling, and the editor backup pattern follows standard conventions.
CRUSH.md (1)
1-87: Comprehensive agent documentation with good structure.The documentation provides thorough guidance on code style, testing procedures, and development workflow. The structure is logical and the content aligns well with modern development practices.
nixie/unittests/test_write_diagram_file.py (1)
8-16: Well-structured unit test with good coverage.The test effectively validates the core functionality of
_write_diagram_file: content writing, path derivation, and file creation. The use of pytest fixtures and clear assertions follows best practices.Makefile (1)
1-43: Well-structured Makefile with comprehensive automation.The Makefile provides excellent build and maintenance automation with proper .PHONY declarations, variable definitions, and helpful documentation. The integration with uv and modern Python tooling is appropriate.
nixie/cli.py (3)
109-137: Well-designed helper function with clear separation of concerns.The function correctly isolates filesystem operations from the rendering workflow, making the code more testable and maintainable. The docstring clearly explains the rationale and parameters.
140-177: Excellent async CLI invocation helper with robust error handling.The function properly manages subprocess execution with semaphore-based concurrency control and provides comprehensive error messages for debugging. The nosemgrep comment appropriately addresses the security scanner warning for subprocess execution.
211-232: Excellent refactoring that simplifies coordination logic.The function now cleanly delegates to focused helper functions and provides comprehensive error handling with user-friendly messages. The use of try-except-else pattern improves code clarity.
nixie/unittests/test_run_mermaid_cli.py (2)
14-45: Comprehensive test coverage for successful CLI invocation.The test effectively verifies command construction, logging, and successful execution using appropriate mocking strategies. The mock functions correctly match the expected signatures even though parameters are unused.
48-79: Thorough test coverage for CLI failure scenarios.The test properly verifies exception raising and ensures error messages contain essential debugging information including the parse error, filename, and command. The assertions effectively validate the error message construction.
1 participant
Summary
_write_diagram_fileand_run_mermaid_clihelpersrender_blockto coordinate helpersTesting
ruff check nixie/cli.py nixie/unittests/test_write_diagram_file.py nixie/unittests/test_run_mermaid_cli.pypyright nixie/cli.py nixie/unittests/test_write_diagram_file.py nixie/unittests/test_run_mermaid_cli.pypython -m pytesthttps://chatgpt.com/codex/tasks/task_e_6893db328e788322a7e0aeb1bd1bf745
Summary by Sourcery
Refactor mermaid diagram rendering by extracting helper functions and adding unit tests for them
Enhancements:
Tests: