fix: prevent crashes when unparsing count, extend, and append actions#406
Open
arpitjain099 wants to merge 1 commit into
Open
fix: prevent crashes when unparsing count, extend, and append actions#406arpitjain099 wants to merge 1 commit into
arpitjain099 wants to merge 1 commit into
Conversation
Three action unparsers raised on valid input reachable through the public get_effective_command_line_invocation(): - _unparse_count_action multiplied the flag body by the stored value. When a count option was not given and its default was None, the value was None and the multiplication raised TypeError. A default of 0 (or a value equal to a nonzero default) produced a bare prefix character like "-" instead of omitting the unused flag. - _unparse_extend_action passed the stored values straight into " ".join, so a typed option (for example type=int) raised TypeError, and unlike the append unparser it never quoted values, so entries containing spaces broke the round trip. - _unparse_append_action indexed values[0] to detect nested lists, which raised IndexError when an append option with default=[] was not given. Each case now emits the correct effective invocation or nothing when the option was not supplied. Added regression tests covering all three. Signed-off-by: arpitjain099 <arpitjain099@gmail.com>
Reviewer's GuideFixes reverse argparse unparsing for count, extend, and append actions to avoid crashes and ensure omitted/typed/space-containing options either round-trip correctly or are not emitted, with matching regression tests added. Sequence diagram for updated action unparsing behavior in get_effective_command_line_invocationsequenceDiagram
actor User
participant ReverseArgumentParser
participant Namespace
User->>ReverseArgumentParser: get_effective_command_line_invocation()
ReverseArgumentParser->>Namespace: getattr(namespace, action.dest)
rect rgb(240,240,240)
Note over ReverseArgumentParser,Namespace: append action
ReverseArgumentParser->>ReverseArgumentParser: _unparse_append_action(action)
ReverseArgumentParser->>Namespace: getattr(self._namespace, action.dest)
alt values not list
ReverseArgumentParser->>ReverseArgumentParser: values = [values]
end
alt values is empty
ReverseArgumentParser-->>ReverseArgumentParser: return (skip _append_list_of_args)
else values non-empty
ReverseArgumentParser->>ReverseArgumentParser: _append_list_of_args(result)
end
end
rect rgb(240,240,240)
Note over ReverseArgumentParser,Namespace: count action
ReverseArgumentParser->>ReverseArgumentParser: _unparse_count_action(action)
ReverseArgumentParser->>Namespace: getattr(self._namespace, action.dest)
alt value is None
ReverseArgumentParser-->>ReverseArgumentParser: return (no flag emitted)
else value is not None
ReverseArgumentParser->>ReverseArgumentParser: count = value - action.default
alt count <= 0
ReverseArgumentParser-->>ReverseArgumentParser: return (no flag emitted)
else count > 0
ReverseArgumentParser->>ReverseArgumentParser: _append_list_of_args([...])
end
end
end
rect rgb(240,240,240)
Note over ReverseArgumentParser,Namespace: extend action
ReverseArgumentParser->>ReverseArgumentParser: _unparse_extend_action(action)
ReverseArgumentParser->>Namespace: getattr(self._namespace, action.dest)
alt values is not None
ReverseArgumentParser->>ReverseArgumentParser: quote_arg_if_necessary(str(value)) for each value
ReverseArgumentParser->>ReverseArgumentParser: _append_list_of_args([option_string, quoted_values])
else values is None
ReverseArgumentParser-->>ReverseArgumentParser: return (no args emitted)
end
end
ReverseArgumentParser-->>User: reconstructed command line
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="reverse_argparse/reverse_argparse.py" line_range="403-404" />
<code_context>
flag = self._get_option_string(action)
if not isinstance(values, list):
values = [values]
+ if not values:
+ return
result = []
if isinstance(values[0], list):
</code_context>
<issue_to_address>
**question:** Empty append lists no longer emit the flag, unlike extend actions; consider whether this asymmetry is intentional.
With this early return, `append` actions whose namespace value is an empty list will no longer emit the flag, whereas `_unparse_extend_action` still emits the flag even when `values` is empty (via `[flag, *values]`). If `append` and `extend` should behave consistently for empty lists, consider emitting the flag without values instead of returning early.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+403
to
+404
| if not values: | ||
| return |
There was a problem hiding this comment.
question: Empty append lists no longer emit the flag, unlike extend actions; consider whether this asymmetry is intentional.
With this early return, append actions whose namespace value is an empty list will no longer emit the flag, whereas _unparse_extend_action still emits the flag even when values is empty (via [flag, *values]). If append and extend should behave consistently for empty lists, consider emitting the flag without values instead of returning early.
Collaborator
There was a problem hiding this comment.
I've reviewed these changes, and they are indeed correct. @sourcery-ai, are you saying we need to make similar changes to _unparse_extend_action as well?
GhostofGoes
approved these changes
Jul 16, 2026
Collaborator
|
praise: This is a fantastic contribution! 🎉 Thanks so much, @arpitjain099, for finding these bugs, building out unit tests to capture them, and then offering up the solution. Your thorough work is very much appreciated. I'll see about getting this merged in soon. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #406 +/- ##
==========================================
+ Coverage 94.01% 95.37% +1.36%
==========================================
Files 2 2
Lines 167 173 +6
Branches 37 40 +3
==========================================
+ Hits 157 165 +8
+ Misses 4 3 -1
+ Partials 6 5 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
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
3 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.
Type: Bug
Description
Three of the action unparsers raise (or emit malformed output) on valid input that reaches them through the public
get_effective_command_line_invocation(). I ran into these while reversing a parser that usedcount,extend, andappendoptions, then reduced each to a minimal repro._unparse_count_actionbuilds the short flag withflag[0] + flag[1] * count. For acountoption that was not given and whose default isNone, the stored value isNoneand the multiplication raisesTypeError: can't multiply sequence by non-int of type 'NoneType'. With a default of0(or a value equal to a nonzero default)countis0, so it emits a bare prefix like-instead of omitting the unused flag._unparse_extend_actionpasses the stored values straight into" ".join(...), so a typed option such astype=intraisesTypeError: sequence item 1: expected str instance, int found. It also never callsquote_arg_if_necessary, unlike theappendunparser, so a value containing spaces is emitted unquoted and the round trip breaks._unparse_append_actionindexesvalues[0]to detect the nested-list case, which raisesIndexError: list index out of rangewhen anappendoption withdefault=[]was not given (thevalues is Noneguard does not catch an empty list).Related Issues/PRs
None that I could find; I came across these while using the library.
Motivation
get_effective_command_line_invocation()should either reproduce the invocation or omit an option that was not supplied. In these three cases it crashes or prints something that would not parse back, which defeats the point of the reconstructed command line.Implementation Details
Each fix is small and local to the action it concerns:
count: return early when the value isNone, and skip emitting anything when the effective count is<= 0(the flag was not given beyond its default).extend: quote each value withquote_arg_if_necessary(str(value)), matching what theappendunparser already does. This fixes both the non-string crash and the unquoted-spaces round trip.append: return early when the normalized value list is empty, before thevalues[0]access.I kept the changes minimal rather than refactoring the shared logic, but happy to consolidate if you would prefer.
Testing
Added regression cases to the existing parametrized tests for all three actions (a not-given
countwithNone/0/nonzero defaults, anextendwithtype=intand one with a space-containing value, and anappendwithdefault=[]that was not given). Confirmed each case failed before the change and passes after.The full suite passes locally on Python 3.12:
ruff check,ruff format --check, andmypyare also clean on both changed files.Documentation
No documentation changes; behavior of the public API is unchanged except that these inputs no longer crash or emit malformed output.
Summary by Sourcery
Prevent crashes and malformed output when unparsing count, extend, and append actions from command-line invocations.
Bug Fixes:
Tests: