Skip to content

Merge file and CLI rendering helpers #13

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
leynos merged 3 commits into from
Aug 7, 2025
Merged

Merge file and CLI rendering helpers #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
fc8acbc
Merge file and CLI rendering helpers
leynos Aug 7, 2025
5656e78
Apply PyUpgrade
leynos Aug 7, 2025
bb13e4e
Remove broad except in render_block and unused import
leynos Aug 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 2 additions & 8 deletions nixie/cli.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import shutil
import sys
import tempfile
import traceback
import typing
from contextlib import contextmanager
from pathlib import Path
Expand Down Expand Up @@ -108,7 +107,7 @@ async def wait_for_proc(
"""Wait for a process to complete and return its success status and stderr."""
try:
_, stderr = await asyncio.wait_for(proc.communicate(), timeout)
except asyncio.TimeoutError:
except TimeoutError:
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Fix timeout exception class to catch asyncio timeouts

Catch asyncio.TimeoutError (raised by asyncio.wait_for), not the built-in TimeoutError. The current clause will fail to intercept timeouts, changing behaviour and leaking processes until the outer scope handles the exception.

-    except TimeoutError:
+    except asyncio.TimeoutError:
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
except TimeoutError:
except asyncio.TimeoutError:
🤖 Prompt for AI Agents 
In nixie/cli.py at line 110, replace the built-in TimeoutError exception with
asyncio.TimeoutError to correctly catch timeouts raised by asyncio.wait_for.
Import asyncio if not already imported, and update the except clause to except
asyncio.TimeoutError to prevent unhandled timeout exceptions and potential
process leaks.

Copy link
Copy Markdown
Owner Author

@leynos leynos Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@coderabbitai new GitHub issue, please

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Aug 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧩 Analysis chain

@leynos I'll create a GitHub issue for the timeout exception fix.

🏁 Script executed:

#!/bin/bash
gh issue create \
  --title "Fix timeout exception handling in wait_for_proc function" \
  --body "## Description

The \`wait_for_proc\` function in \`nixie/cli.py\` currently catches the built-in \`TimeoutError\` instead of \`asyncio.TimeoutError\`. This prevents proper handling of timeouts raised by \`asyncio.wait_for\`, potentially leading to unhandled exceptions and process leaks.

## Expected Behaviour
The function should catch \`asyncio.TimeoutError\` to properly handle timeouts from \`asyncio.wait_for\`.

## Current Behaviour
The function catches the built-in \`TimeoutError\`, which will not intercept timeouts from \`asyncio.wait_for\`.

## Fix
Change line 110 from:
\`\`\`python
except TimeoutError:
\`\`\`

to:
\`\`\`python
except asyncio.TimeoutError:
\`\`\`

## References
- PR: https://github.com/leynos/nixie/pull/13
- Comment: https://github.com/leynos/nixie/pull/13#discussion_r2261636969

Requested by: @leynos" \
  --assignee leynos

Length of output: 959

@leynos I've successfully created GitHub issue #15 for the timeout exception handling fix. The issue includes:

  • Clear description of the problem with the incorrect exception type
  • Expected vs current behaviour
  • The specific code change needed
  • References to this PR and comment thread
  • Assignment to you as requested

You can view the issue at: #15

🧬 Code Graph Analysis Results

Relevant snippet from nixie/unittests/test_get_mmdc_cmd.py (lines 22-25)

def which(cmd: str) -> str | None:
        if cmd == "bun":
            return "/usr/bin/bun"
        return None

This snippet defines a mock which function that returns a path for the "bun" command and None otherwise. It is relevant because the nixie/cli.py file uses shutil.which to detect the availability of CLI executables (mmdc, bun, npx) in the get_mmdc_cmd function. This mock could be part of tests that simulate the presence or absence of these executables.

proc.kill()
await proc.wait()
print(f"{path}: diagram {idx} timed out", file=sys.stderr)
Expand Down Expand Up @@ -212,16 +211,11 @@ async def render_block(
except FileNotFoundError as exc:
cli = exc.filename or "mmdc"
print(
"Error: '{0}' not found. Install Node.js with npx or Bun to use @mermaid-js/mermaid-cli.".format(
cli
),
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
Expand Down
1 change: 0 additions & 1 deletion nixie/unittests/test_render_diagram.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
from __future__ import annotations

import asyncio
import logging
import shlex
Expand Down