GGraziadei commented on PR #8971:
URL: https://github.com/apache/storm/pull/8971#issuecomment-5219145889
Hi @sercuzz8,
Thanks for tackling this, the diagnosis is right, and reconstructing the
order from `sys.argv` is the correct shape for the fix. A few things I'd like
addressed before this goes in.
- The assignment replaces `main_args` instead of extending it. The previous
line was `main_args += unknown_args`; the new one overwrites `main_args` with
only the tokens found in `argv`. Anything in `main_args` that didn't come
verbatim from the command line, an argparse `default=[...]` on a subparser
positional, or values injected before `main()` reassembles, is now silently
dropped. Could you confirm no subcommand relies on that?
- Tokens can be lost with no diagnostic.There's no `else` branch and no
check that `len(merged) == len(known_args) + len(unknown_args)`. If the
invariant ever breaks, `storm jar` launches a topology with arguments missing
and no error, it just looks like it worked. Either raise:
```python
if len(merged) != len(known_args) + len(unknown_args):
raise ValueError("could not reconstruct argument order from sys.argv")
```
or fall back to the old concatenation, if you'd rather not add a new failure
path to a released CLI.
- Consider dropping the positional pointers. The two indices tie the
function to both lists being ordered subsequences of `argv`. A count-based
filter is shorter, order-agnostic, and gives you the leftover check for free:
```python
from collections import Counter
remaining = Counter(known_args) + Counter(unknown_args)
merged = []
for token in sys_args:
if remaining[token]:
remaining[token] -= 1
merged.append(token)
```
**Nits:** the `#Recombine ...` line should be a docstring with """
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
