Skip to content

Reinstate ordering of arguments in bin/storm.py - #8971

Open
sercuzz8 wants to merge 1 commit into
apache:masterfrom
sercuzz8:fix-argument-order
Open

Reinstate ordering of arguments in bin/storm.py#8971
sercuzz8 wants to merge 1 commit into
apache:masterfrom
sercuzz8:fix-argument-order

Conversation

@sercuzz8

@sercuzz8 sercuzz8 commented Aug 6, 2026

Copy link
Copy Markdown

Summary

argparse's parse_known_args() splits main_args (nargs='*') from unrecognized flags
(unknown_args), losing their relative order when they are interleaved on the command line.
This breaks down the flow of mvn clean install, thus the order must be reinstated.

Following this discussion

@GGraziadei GGraziadei changed the title [Fix] Reinstate ordering of arguments Reinstate ordering of arguments in bin/storm.py Aug 7, 2026
@GGraziadei

Copy link
Copy Markdown
Member

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:

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:
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 """

@GGraziadei
GGraziadei self-requested a review August 7, 2026 15:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants