Handle success banner encoding fallback on Windows - #29
Conversation
Reviewer's GuideEnhance CLI success banner with an encoding-aware fallback, integrate it into the output logic, and verify behavior through parameterized integration tests. Sequence diagram for CLI success banner output with encoding fallbacksequenceDiagram
participant main as "main()"
participant stdout as "sys.stdout (_EncodingAwareStream)"
participant banner as "resolve_success_banner()"
main->>banner: resolve_success_banner(stdout)
alt stdout encoding supports emoji
banner-->>main: SUCCESS_BANNER
else encoding does not support emoji
banner-->>main: ASCII_SUCCESS_BANNER
end
main->>stdout: print(banner, flush=True)
Class diagram for encoding-aware success banner selectionclassDiagram
class _EncodingAwareStream {
encoding: str | None
}
class cli {
+SUCCESS_BANNER: str
+ASCII_SUCCESS_BANNER: str
+resolve_success_banner(stream: _EncodingAwareStream | None): str
}
_EncodingAwareStream <|.. cli: 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. Summary by CodeRabbit
WalkthroughIntroduce encoding-aware success banner selection in the CLI: add Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as User
participant CLI as CLI main
participant Resolver as resolve_success_banner
participant Stdout as sys.stdout
U->>CLI: Run command
CLI->>Resolver: resolve_success_banner(Stdout)
Resolver->>Stdout: Read Stdout.encoding
alt encoding supports emoji
Resolver-->>CLI: SUCCESS_BANNER (emoji)
else encoding missing/doesn't support emoji
Resolver-->>CLI: ASCII_SUCCESS_BANNER
end
CLI->>Stdout: Print chosen banner
note right of Stdout: Ensure printed banner is representable\nby the stream encoding
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used📓 Path-based instructions (3)**/*.py📄 CodeRabbit inference engine (AGENTS.md)
Files:
⚙️ CodeRabbit configuration file
Files:
tests/integration/test_*.py📄 CodeRabbit inference engine (.rules/python-00.md)
Files:
{**/unittests/test_*.py,tests/integration/test_*.py}📄 CodeRabbit inference engine (.rules/python-00.md)
Files:
🧬 Code graph analysis (2)tests/integration/test_cli_behavior.py (2)
tests/unit/test_cli.py (1)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Ruff (0.13.1)tests/integration/test_cli_behavior.py�[1;31mruff failed�[0m tests/unit/test_cli.py�[1;31mruff failed�[0m Warning Review ran into problems🔥 ProblemsErrors were encountered while retrieving linked issues. Errors (1)
Comment |
![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.
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `nixie/cli.py:122-124` </location>
<code_context>
+ encoding = getattr(stream, "encoding", None)
+ if not encoding:
+ return SUCCESS_BANNER
+ try:
+ SUCCESS_BANNER.encode(encoding)
+ except UnicodeEncodeError:
+ return ASCII_SUCCESS_BANNER
+ return SUCCESS_BANNER
</code_context>
<issue_to_address>
**suggestion:** Consider catching broader encoding errors, not just UnicodeEncodeError.
Other exceptions like LookupError may also occur during encoding. Consider handling these to improve robustness.
</issue_to_address>
### Comment 2
<location> `tests/integration/test_cli_behavior.py:276-281` </location>
<code_context>
assert len(end_markers) == 1, "Expected one end marker despite the failure"
+
+
+@pytest.mark.parametrize(
+ ("stream", "expected"),
+ [
+ (SimpleNamespace(encoding="utf-8"), SUCCESS_BANNER),
+ (SimpleNamespace(encoding="cp1252"), ASCII_SUCCESS_BANNER),
+ (SimpleNamespace(encoding=None), SUCCESS_BANNER),
+ (None, SUCCESS_BANNER),
+ ],
+)
+def test_resolve_success_banner_handles_non_utf8_streams(
+ stream: SimpleNamespace | None, expected: str
+) -> None:
</code_context>
<issue_to_address>
**suggestion (testing):** Consider adding a test case for streams with unknown or invalid encodings.
Adding a test with an invalid encoding (e.g., 'unknown-encoding') will help verify that the fallback logic works correctly and handles unexpected values gracefully.
```suggestion
[
(SimpleNamespace(encoding="utf-8"), SUCCESS_BANNER),
(SimpleNamespace(encoding="cp1252"), ASCII_SUCCESS_BANNER),
(SimpleNamespace(encoding=None), SUCCESS_BANNER),
(None, SUCCESS_BANNER),
(SimpleNamespace(encoding="unknown-encoding"), SUCCESS_BANNER),
],
```
</issue_to_address>
### Comment 3
<location> `nixie/cli.py:107` </location>
<code_context>
+ASCII_SUCCESS_BANNER: typ.Final[str] = "All diagrams validated successfully!"
+
+
+def resolve_success_banner(stream: _EncodingAwareStream | None) -> str:
+ """Return a stream-compatible success banner.
+
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural and unit tests for the new resolve_success_banner function.
The resolve_success_banner function is a new feature that determines which success banner to display based on stream encoding. You must add both behavioural and unit tests to verify its correct operation, including cases for UTF-8, cp1252, and None encodings.
<details>
<summary>Review instructions:</summary>
**Path patterns:** `**/*`
**Instructions:**
For any new feature or change to an existing feature, both behavioural *and* unit tests are required.
</details>
</issue_to_address>
### Comment 4
<location> `nixie/cli.py:593` </location>
<code_context>
print(f"<== {path}")
if all_success:
- print(SUCCESS_BANNER, flush=True)
+ print(resolve_success_banner(sys.stdout), flush=True)
return 0 if all_success else 1
</code_context>
<issue_to_address>
**issue (review_instructions):** Add behavioural tests for CLI output with different stream encodings.
Since the CLI now conditionally prints different banners based on stream encoding, you must add behavioural tests that exercise this logic, ensuring the correct banner is printed for various encodings.
<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.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/integration/test_cli_behavior.py (2)
109-113: Make success‑path assertion encoding‑agnostic.The CLI now prints either the emoji or ASCII banner depending on sys.stdout.encoding. The current assertion fails on Windows/cp1252.
Apply this diff:
- if expected_exit == 0: - assert captured.out.count(SUCCESS_BANNER) == 1 - else: - assert captured.out.count(SUCCESS_BANNER) == 0 + if expected_exit == 0: + assert any( + captured.out.count(banner) == 1 + for banner in (SUCCESS_BANNER, ASCII_SUCCESS_BANNER) + ) + else: + assert captured.out.count(SUCCESS_BANNER) == 0 + assert captured.out.count(ASCII_SUCCESS_BANNER) == 0As per coding guidelines
274-289: Add a case for unknown encodings to validate robustness.Exercise the LookupError path to ensure a safe ASCII fallback once implemented.
Apply this diff after hardening the resolver:
@pytest.mark.parametrize( ("stream", "expected"), [ (SimpleNamespace(encoding="utf-8"), SUCCESS_BANNER), (SimpleNamespace(encoding="cp1252"), ASCII_SUCCESS_BANNER), (SimpleNamespace(encoding=None), SUCCESS_BANNER), (None, SUCCESS_BANNER), + (SimpleNamespace(encoding="not-an-encoding"), ASCII_SUCCESS_BANNER), ], ) def test_resolve_success_banner_handles_non_utf8_streams( stream: SimpleNamespace | None, expected: str ) -> None: """Prefer the celebratory banner but fall back when encoding rejects it.""" assert resolve_success_banner(stream) == expectedAs per coding guidelines
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
nixie/cli.py(3 hunks)tests/integration/test_cli_behavior.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Python changes must pass tests (unit and behavioral) before completion/commit
Python code must pass lint checks (make lint)
Python code must adhere to formatting standards (make check-fmt/make fmt)
Python code must pass type checking (make typecheck)
Follow core Python 3.13 style conventions per .rules/python-00.md
Apply best practices for context managers per .rules/python-context-managers.md
Follow generator and iterator patterns per .rules/python-generators.md
Follow function return conventions per .rules/python-return.md
Apply Python typing best practices per .rules/python-typing.md
**/*.py: Name Python files in snake_case (e.g., http_client.py, task_queue.py)
Classes must use PascalCase
Variables and functions must use snake_case
Module-level constants use UPPER_SNAKE_CASE
Prefix non-exported helpers or internal APIs with a single underscore
Use typing everywhere; maintain full static type coverage with Pyright
Use TypedDict or @DataClass for structured data; prefer @DataClass(slots=True) for internal-only
Avoid Any; use Unknown, generics, or cast() with justification if Any is used
Be explicit with return types for all public functions and class methods (e.g., -> None, -> str)
Favor immutability (prefer tuples to lists; use frozendict or types.MappingProxyType where appropriate)
Use# pyright: ignoresparingly and include an explanation when used
Avoid side effects at import time; modules should not modify global state or perform actions on import
Never hardcode secrets in source code
Write NumPy-style docstrings for public functions, classes, and modules
Add inline comments to explain non-obvious logic or decisions
**/*.py: Use context managers to encapsulate setup/teardown for resources (files, locks, connections) instead of manual management
Use @contextmanager from contextlib for straightforward, linear setup/teardown without persistent internal state
Implement a class-based context manager (enter/exit) when there is internal sta...
Files:
nixie/cli.pytests/integration/test_cli_behavior.py
⚙️ CodeRabbit configuration file
**/*.py: - Keep cyclomatic complexity ≤ 12
- Follow single responsibility and CQRS (command/query segregation)
- Docstrings must follow the
numpystyle 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 / -yse / -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 byFIXME:or a ticket link, and used only as a last resort.- Use
pytestfixtures for shared setup (conftest.pyorfixtures/)- Replace duplicate tests with
@pytest.mark.parametrize- Prefer
pytest-mockorunittest.mockfor 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/caseor 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.- All path manipulation must be performed using pathlib for cross platform safety. Do not use string manipulation for extraction of path components or filename elements.
Files:
nixie/cli.pytests/integration/test_cli_behavior.py
tests/integration/test_*.py
📄 CodeRabbit inference engine (.rules/python-00.md)
Place integration tests under tests/integration/ with files prefixed with test_
Files:
tests/integration/test_cli_behavior.py
{**/unittests/test_*.py,tests/integration/test_*.py}
📄 CodeRabbit inference engine (.rules/python-00.md)
{**/unittests/test_*.py,tests/integration/test_*.py}: Use pytest idioms: prefer fixtures, 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
Avoid excessive mocking; use doubles only for external services or non-deterministic behaviour
Files:
tests/integration/test_cli_behavior.py
🧬 Code graph analysis (1)
tests/integration/test_cli_behavior.py (1)
nixie/cli.py (3)
cli(628-645)main(572-594)resolve_success_banner(107-126)
⏰ Context from checks skipped due to timeout of 120000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Sourcery review
🔇 Additional comments (5)
nixie/cli.py (3)
82-86: Keep Protocol type under TYPE_CHECKING; good runtime footprint.Avoid importing Protocol at runtime; this pattern is correct.
103-105: Add ASCII fallback constant; good default for legacy consoles.The banner text is stable and suitable for tests.
593-593: Route final output through resolver; correct behaviour.Print via resolve_success_banner(sys.stdout) to avoid UnicodeEncodeError on Windows.
tests/integration/test_cli_behavior.py (2)
4-4: Import SimpleNamespace for stream doubles; fine.The test double matches the Protocol surface needed by the resolver.
9-15: Import new public APIs; fine.The tests import only public symbols exposed by nixie.cli.
1 participant
Summary
Testing
https://chatgpt.com/codex/tasks/task_e_68d6aca53f6083228b7c79965eb82d1f
Summary by Sourcery
Provide an ASCII fallback for the CLI success banner on environments with non-UTF-8 encodings and expose the resolution logic for integration testing
New Features:
Enhancements:
Tests: