[PR] [Fix] Reinstate ordering of arguments (storm)

["sercuzz8 (via GitHub)" <gi...@apache.org>]

sercuzz8 opened a new pull request, #8971:
URL: https://github.com/apache/storm/pull/8971

   ## 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.


-- 
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: issues-unsubscribe@storm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Reinstate ordering of arguments in `bin/storm.py` (storm)

["GGraziadei (via GitHub)" <gi...@apache.org>]

GGraziadei merged PR #8971:
URL: https://github.com/apache/storm/pull/8971


-- 
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: issues-unsubscribe@storm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Reinstate ordering of arguments in `bin/storm.py` (storm)

["GGraziadei (via GitHub)" <gi...@apache.org>]

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: issues-unsubscribe@storm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

Re: [PR] Reinstate ordering of arguments in `bin/storm.py` (storm)

["sercuzz8 (via GitHub)" <gi...@apache.org>]

sercuzz8 commented on PR #8971:
URL: https://github.com/apache/storm/pull/8971#issuecomment-5226570773

   Hello @GGraziadei,
   thank you for the pieces of advice.
   1. I had overlooked the case, but I have now checked. `add_common_options` (`bin/storm.py:385-389`) declares `main_args` with `nargs='*'` and no `default=`, so it never holds  synthetic values from argparse. Only  `local()` (`bin/storm.py:1065`) prepend a non-CLI value in `main_args` and that runs *after* `raw_args.func(raw_args)` (and consequently the introduced method). No subcommand relies on a pre-merge injected default, so the replace semantics are safe;
   2. The proposed solution manages to be more elegant, concise and complete than the original one, implemented;
   3. It now falls back to the original functioning as this behaviour was accepted before;
   4. A new docstring with more information regarding input and output has been added. Consider accepting or rejecting it based on verbosity.


-- 
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: issues-unsubscribe@storm.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org