fix: Raise WorkflowError on empty file path in _IOFile.check#3769
Conversation
📝 WalkthroughWalkthroughIntroduces an early validation in _IOFile.check to raise WorkflowError when a file path is an empty string. Removes unused ABC-related imports. Updates a conda-related test to use the conda-forge channel and assert a successful subprocess return code. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Workflow as Workflow
participant IOFile as _IOFile.check
User->>Workflow: Define rule with output path
Workflow->>IOFile: check(path)
alt path is empty string
IOFile-->>Workflow: raise WorkflowError (early)
Workflow-->>User: Error surfaced
else valid path
IOFile-->>Workflow: proceed
Workflow-->>User: Continue execution
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches❌ Failed checks (3 warnings)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.py⚙️ CodeRabbit configuration file
Files:
🪛 Ruff (0.13.2)tests/tests_using_conda.py327-327: (S602) 328-328: Starting a process with a partial executable path (S607) ⏰ 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). (44)
🔇 Additional comments (1)
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/snakemake/io/__init__.py (1)
459-463: LGTM! Excellent fix for issue #341.The empty-string check correctly prevents the cryptic IndexError downstream by raising a clear, actionable WorkflowError early. The placement after the callable check is correct, and passing
rule=self.ruleprovides helpful context.Optional style note: Ruff suggests avoiding long messages outside the exception class (TRY003). While the message is clear and not excessively long, you could optionally extract it to a constant or use a shorter inline message if you want to align with this guideline. However, this is a minor style preference and not a blocker.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/snakemake/io/__init__.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
src/snakemake/io/__init__.py
🪛 Ruff (0.13.2)
src/snakemake/io/__init__.py
460-463: Avoid specifying long messages outside the exception class
(TRY003)
⏰ 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). (34)
- GitHub Check: tests (9, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py311)
- GitHub Check: tests (10, windows-2022, py313)
- GitHub Check: tests (7, windows-2022, py313)
- GitHub Check: tests (9, ubuntu-latest, py312)
- GitHub Check: tests (8, ubuntu-latest, py312)
- GitHub Check: tests (8, windows-2022, py313)
- GitHub Check: tests (7, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py313)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (8, ubuntu-latest, py311)
- GitHub Check: tests (6, windows-2022, py313)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py313)
- GitHub Check: tests (4, windows-2022, py313)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py313)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-2022, py313)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (4, ubuntu-latest, py313)
- GitHub Check: tests (2, windows-2022, py313)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (3, windows-2022, py313)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: tests (2, ubuntu-latest, py313)
- GitHub Check: tests (1, macos-latest, py313)
- GitHub Check: tests (1, ubuntu-latest, py313)
- GitHub Check: tests (1, windows-2022, py313)
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/main.yml (1)
50-113: Restore the full CI matrix and test suite.This change collapses the matrix to a single Ubuntu/py313 run and limits pytest to
-k test_issue_3192, effectively dropping all other OS/Python combinations and most of the test suite. That guts our CI coverage and will let regressions slip through. Please bring back the previous matrix breadth and full test run before merging.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/main.yml(2 hunks)src/snakemake/dag.py(1 hunks)src/snakemake/deployment/conda.py(2 hunks)tests/test_prebuilt_conda_script/env.yaml(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/test_prebuilt_conda_script/env.yaml
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
src/snakemake/deployment/conda.pysrc/snakemake/dag.py
🪛 GitHub Actions: CI
src/snakemake/deployment/conda.py
[error] 178-178: WorkflowError: Error exporting conda environment --name '/home/runner/work/snakemake/snakemake/.pixi/envs/py313/envs/test_issue3192': CondaValueError: Environment names cannot contain path separators.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/snakemake/deployment/conda.py(1 hunks)src/snakemake/rules.py(1 hunks)tests/tests_using_conda.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/snakemake/deployment/conda.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
src/snakemake/rules.pytests/tests_using_conda.py
🪛 GitHub Actions: CI
src/snakemake/rules.py
[error] 1205-1205: AssertionError: CondaEnvSpecType.from_spec(conda_env) did not yield DIR as expected. Expected a directory-based Conda env spec, but got a different spec type (NAME). This causes conda environment expansion to fail during DAG initialization.
🪛 Ruff (0.13.2)
tests/tests_using_conda.py
327-327: Starting a process with a partial executable path
(S607)
⏰ 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: apidocs
🔇 Additional comments (1)
tests/tests_using_conda.py (1)
327-327: LGTM! Auto-confirm flag improves test reliability.The addition of
-yto auto-confirm the conda environment creation is appropriate for non-interactive test execution.Note: The static analysis warning (S607) about partial executable path is a false positive in this test context where the command is hardcoded and controlled.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/snakemake/deployment/conda.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
⚙️ CodeRabbit configuration file
**/*.py: Do not try to improve formatting.
Do not suggest type annotations for functions that are defined inside of functions or methods.
Do not suggest type annotation of theselfargument of methods.
Do not suggest type annotation of theclsargument of classmethods.
Do not suggest return type annotation if a function or method does not contain areturnstatement.
Files:
src/snakemake/deployment/conda.py
🧬 Code graph analysis (1)
src/snakemake/deployment/conda.py (1)
src/snakemake/common/__init__.py (1)
is_local_file(127-128)
🪛 GitHub Actions: CI
src/snakemake/deployment/conda.py
[error] 1022-1022: Conda environment spec path is not a directory: '/home/runner/work/snakemake/snakemake/.pixi/envs/py313/envs/test_issue3192'. The code asserts the path exists; environment not created, leading to AssertionError during CondaEnvSpecType.from_spec.
🪛 Ruff (0.13.2)
src/snakemake/deployment/conda.py
1021-1021: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
|
Done! If conda install without channels selected, it will fail. |
|
Great work! |
🤖 I have created a release *beep* *boop* --- ## [9.11.9](v9.11.8...v9.11.9) (2025-10-03) ### Bug Fixes * Raise WorkflowError on empty file path in _IOFile.check ([#3769](#3769)) ([4249ff7](4249ff7)) * remove id field in notebook cells created by nbformat, thereby increasing compatibility with different jupyter versions ([#3767](#3767)) ([46cf3ed](46cf3ed)) * show failed logs in generated unit tests ([#3771](#3771)) ([61f4e9b](61f4e9b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
…ke#3769) fix snakemake#341 Now the file with empty string will never pass the check. Or should this be a warning instead of a WorkflowError? ### QC <!-- Make sure that you can tick the boxes below. --> * [ ] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [ ] The documentation (`docs/`) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake). <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Added an early, clear error when an empty file path is provided, preventing downstream confusion. * **Tests** * Updated a conda-related test to use the conda-forge channel and assert successful execution. * **Chores** * Removed unused imports to reduce overhead. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
🤖 I have created a release *beep* *boop* --- ## [9.11.9](snakemake/snakemake@v9.11.8...v9.11.9) (2025-10-03) ### Bug Fixes * Raise WorkflowError on empty file path in _IOFile.check ([snakemake#3769](snakemake#3769)) ([4249ff7](snakemake@4249ff7)) * remove id field in notebook cells created by nbformat, thereby increasing compatibility with different jupyter versions ([snakemake#3767](snakemake#3767)) ([46cf3ed](snakemake@46cf3ed)) * show failed logs in generated unit tests ([snakemake#3771](snakemake#3771)) ([61f4e9b](snakemake@61f4e9b)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
fix #341
Now the file with empty string will never pass the check.
Or should this be a warning instead of a WorkflowError?
QC
docs/) is updated to reflect the changes or this is not necessary (e.g. if the change does neither modify the language nor the behavior or functionalities of Snakemake).Summary by CodeRabbit
Bug Fixes
Tests
Chores