Mark the parsing loop shift as unreachable for shellcheck when it really is - #210
Open
gdevenyi wants to merge 1 commit into
Open
Mark the parsing loop shift as unreachable for shellcheck when it really is#210gdevenyi wants to merge 1 commit into
gdevenyi wants to merge 1 commit into
Conversation
If a script has no positional arguments and all of its optional arguments are actions (such as --help), every branch of the case statement in parse_commandline terminates the script, so the shift that follows is unreachable and shellcheck reports SC2317. Generate a shellcheck directive in front of the shift in exactly that case, and drop the waivers the affected test templates carried. Fixes matejak#204 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017DxhC3HmTM7TQUzkBZnyH2
There was a problem hiding this comment.
🟢 Approval recommended
No unresolved review comments remain, and the targeted changes are covered by existing tests.
Pull request overview
Updates Argbash generation to suppress ShellCheck SC2317 only when the parsing-loop shift is unreachable.
Changes:
- Detects action-only argument sets with no positional arguments.
- Emits a targeted SC2317 suppression.
- Removes obsolete test waivers.
- Documents the fix.
File summaries
| File | Summary |
|---|---|
tests/regressiontests/test-progs.m4 |
Removes obsolete waiver. |
tests/regressiontests/test-prog.m4 |
Removes obsolete waiver. |
tests/regressiontests/test-env-simple.m4 |
Removes obsolete waiver. |
tests/regressiontests/test-env-base.m4 |
Removes obsolete waiver. |
src/stuff.m4 |
Adds parsing-loop reachability detection. |
src/function_generators.m4 |
Emits conditional SC2317 suppression. |
ChangeLog |
Documents the bug fix. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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
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.
Fixes #204
If a script has no positional arguments and all of its optional arguments are actions (such as
--help), every branch of thecasestatement inparse_commandlineterminates the script, so theshiftthat follows is unreachable and shellcheck reports SC2317.This adds
_IF_PARSING_LOOP_SHIFT_IS_UNREACHABLEand generates a# shellcheck disable=SC2317directive in front of theshiftin exactly that case. Scripts with any positional, value, boolean or incremental argument are generated exactly as before.The four test templates that carried a manual SC2317 waiver for this reason had the waiver removed, so the existing tests (which run shellcheck on the generated scripts) now cover the fix.
🤖 Generated with Claude Code
https://claude.ai/code/session_017DxhC3HmTM7TQUzkBZnyH2