Fix CLI one-sided range and resolution options#90
Merged
Conversation
📝 WalkthroughWalkthroughThis PR resolves issue ChangesValidation for Paired Resolution and Open-Ended Frame Ranges
Sequence DiagramsequenceDiagram
participant CliUser as CLI User
participant ConvertCmd as convert_exr_sequence<br/>Command
participant ValidateRes as validate_paired_<br/>resolution
participant ConfigBuilder as ConversionConfigBuilder
participant ApplyRange as apply_frame_range
participant RenderKit as RenderKit API
CliUser->>ConvertCmd: --width 1920 (no --height)
ConvertCmd->>ValidateRes: validate_paired_resolution(1920, None)
ValidateRes-->>ConvertCmd: click.UsageError
ConvertCmd-->>CliUser: exit code 2, error message
CliUser->>ConvertCmd: --width 1920 --height 1080 --start-frame 100
ConvertCmd->>ValidateRes: validate_paired_resolution(1920, 1080)
ValidateRes-->>ConvertCmd: OK
ConvertCmd->>ConfigBuilder: with_resolution(1920, 1080)
ConfigBuilder-->>ConvertCmd: builder
ConvertCmd->>ApplyRange: apply_frame_range(builder, 100, None)
ApplyRange->>ConfigBuilder: with_frame_range(100, None)
ConfigBuilder-->>ApplyRange: builder
ConvertCmd->>RenderKit: convert_with_config(config)
RenderKit-->>ConvertCmd: result (frames 100 to end)
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_api.py (1)
29-30: 💤 Low valueOptional: Add a docstring or comment to satisfy the static analysis hint.
The empty
convertmethod is intentional for this test stub, but SonarCloud flags it. Adding a brief docstring would silence the linter without changing behavior.📝 Suggested docstring
def convert(self, show_progress=None) -> None: + """Stub implementation for testing.""" pass🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/test_api.py` around lines 29 - 30, Add a short docstring to the test stub method convert(self, show_progress=None) -> None explaining that the method is intentionally empty for testing (e.g., "Intentional no-op for test stub; SonarCloud suppression"). This satisfies static analysis without changing behavior; update the convert function's body to contain only the docstring and keep the signature and no-op behavior.Source: MCP tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@tests/test_api.py`:
- Around line 29-30: Add a short docstring to the test stub method convert(self,
show_progress=None) -> None explaining that the method is intentionally empty
for testing (e.g., "Intentional no-op for test stub; SonarCloud suppression").
This satisfies static analysis without changing behavior; update the convert
function's body to contain only the docstring and keep the signature and no-op
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c2491dbf-82d0-4603-b0a0-c62c1093b9b9
📒 Files selected for processing (9)
src/renderkit/api/processor.pysrc/renderkit/cli/batch.pysrc/renderkit/cli/conversion.pysrc/renderkit/cli/main.pysrc/renderkit/core/config.pytests/test_api.pytests/test_batch.pytests/test_cli.pytests/test_converter.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
--width/--heightoptions before conversion side effects--start-frame/--end-frameranges as open-ended boundsRoot cause
The CLI only applied resolution when both dimensions were provided, so a lone dimension was silently dropped. Frame range handling also coerced one-sided ranges into surprising closed ranges, such as
--start-frame NbecomingN-N.Validation
uv --native-tls run --extra dev pytest tests/test_api.py tests/test_cli.py tests/test_batch.py tests/test_converter.pyuv --native-tls run --extra dev ruff check src\renderkit\api\processor.py src\renderkit\cli\conversion.py src\renderkit\cli\main.py src\renderkit\cli\batch.py src\renderkit\core\config.py tests\test_api.py tests\test_cli.py tests\test_batch.py tests\test_converter.pyuv --native-tls run --extra dev pytestuv --native-tls run --extra dev mypyuv --native-tls run --extra dev ruff format --check src\renderkit\api\processor.py src\renderkit\cli\conversion.py src\renderkit\cli\main.py src\renderkit\cli\batch.py src\renderkit\core\config.py tests\test_api.py tests\test_cli.py tests\test_batch.py tests\test_converter.pygit diff --checkCloses #86
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
Tests