chore: dbg sh fail#3447
Conversation
📝 WalkthroughWalkthroughThe script located in Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/System
participant QSUB as Script (qsub)
participant Shell as ShellScript
User ->> QSUB: Call with argument ($1)
QSUB ->> QSUB: Echo $1
QSUB ->> Shell: Execute script specified by $1
sequenceDiagram
participant Workflow as Workflow
participant SpawnJobs as SpawnJobs
Workflow ->> SpawnJobs: Request storage provider setting items
SpawnJobs ->> SpawnJobs: Check if value is callable
alt Value is callable
SpawnJobs -->> Workflow: Raise WorkflowError
else Value is not callable
SpawnJobs -->> Workflow: Return setting items
end
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_issue860/qsub (1)
7-7: Enhance Debug Output RobustnessThe addition of
echo $1is useful for debugging purposes by printing the provided argument before executing it. However, to avoid potential word-splitting or misinterpretation when$1contains spaces or special characters, please consider quoting it (e.g.,echo "$1").
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_issue860/qsub(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (21)
- GitHub Check: tests (10, windows-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py312)
- GitHub Check: tests (10, ubuntu-latest, py311)
- GitHub Check: tests (6, ubuntu-latest, py312)
- GitHub Check: tests (6, ubuntu-latest, py311)
- GitHub Check: tests (5, windows-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py312)
- GitHub Check: tests (5, ubuntu-latest, py311)
- GitHub Check: tests (4, windows-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py312)
- GitHub Check: tests (4, ubuntu-latest, py311)
- GitHub Check: tests (3, windows-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py312)
- GitHub Check: tests (3, ubuntu-latest, py311)
- GitHub Check: tests (2, windows-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py312)
- GitHub Check: tests (2, ubuntu-latest, py311)
- GitHub Check: tests (1, windows-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py312)
- GitHub Check: tests (1, ubuntu-latest, py311)
- GitHub Check: apidocs
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
CHANGELOG.mdis excluded by!CHANGELOG.md
📒 Files selected for processing (1)
src/snakemake/spawn_jobs.py(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.py`: Do not try to improve formatting. Do not suggest ...
**/*.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.
src/snakemake/spawn_jobs.py
🪛 Ruff (0.8.2)
src/snakemake/spawn_jobs.py
61-61: Function definition does not bind loop variable plugin_name
(B023)
🔇 Additional comments (1)
src/snakemake/spawn_jobs.py (1)
14-14: LGTM: Import added for WorkflowErrorThis import is appropriately added to support the new error handling in the
_get_storage_provider_setting_itemsmethod.
| if callable(value): | ||
| raise WorkflowError( | ||
| f"Invalid setting for plugin {plugin_name}. Unable " | ||
| "to pass callable value as a setting to spawned jobs." | ||
| ) |
There was a problem hiding this comment.
Fix the potential closure issue with the plugin_name variable
The added check that prevents callable values from being passed as settings is valuable, but there's a potential issue: the nested fmt_value function uses the loop variable plugin_name in the error message, which is flagged by the static analyzer as B023 (function definition does not bind loop variable).
When the function is called later, it may use the last value of plugin_name from the loop instead of the value at the time of function definition.
To fix this, pass plugin_name as a parameter to the function:
- def fmt_value(tag, value):
+ def fmt_value(tag, value, current_plugin=plugin_name):
if callable(value):
raise WorkflowError(
- f"Invalid setting for plugin {plugin_name}. Unable "
+ f"Invalid setting for plugin {current_plugin}. Unable "
"to pass callable value as a setting to spawned jobs."
)📝 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.
| if callable(value): | |
| raise WorkflowError( | |
| f"Invalid setting for plugin {plugin_name}. Unable " | |
| "to pass callable value as a setting to spawned jobs." | |
| ) | |
| def fmt_value(tag, value, current_plugin=plugin_name): | |
| if callable(value): | |
| raise WorkflowError( | |
| f"Invalid setting for plugin {current_plugin}. Unable " | |
| "to pass callable value as a setting to spawned jobs." | |
| ) |
🧰 Tools
🪛 Ruff (0.8.2)
61-61: Function definition does not bind loop variable plugin_name
(B023)
### Description <!--Add a description of your PR here--> ### QC <!-- Make sure that you can tick the boxes below. --> * [x] The PR contains a test case for the changes or the changes are already covered by an existing test case. * [x] 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 - **New Features** - The script now displays the initial input parameter before execution, offering clearer feedback on its usage. - **Bug Fixes** - Enhanced error handling by preventing callable values from being used as settings in spawned jobs. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Description
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