Forward unmodified ARGV to subcommand #631
Conversation
Avoid altering original `ARGV` when combining it with possible `SHARDS_OPTS` variable and pass them verbatim to the subcommand. Introduce a naive integration test for subcommand to validate the change works correctly.
Avoid OptionParser to short-circuit `--help` and return immediately by setting a flag for it and evaluating at the end of the processing of unknown options. This is only done for the CLI invocation and is not part of Shards module (as the help and usage options are only available in this context).
|
I like how Git, for example, distinguishes between general and command arguments: those before the command name are general git arguments, while those after the command name will all be forwarded to the actual command. Applied to Shards:
|
|
@ysbaddaden I like that too, even it mentions Off-topic: I found OptionParser quite limited on that, but from my personal point of view, most of the other shards out there are too overwhelming (aka: everything but the kitchen sink). Unsure if I should back off from the changes in commit 5a6dc6c and leave it for Shards main program to respond to Open to suggestions! Thank you for looking into it! |
Follow the pattern used in other tests and use simple `sh` script on non-Windows platforms.
src/cli.cr
Outdated
| @@ -2,6 +2,8 @@ require "option_parser" | |||
| require "./commands/*" | |||
|
|
|||
| module Shards | |||
| class_property? display_help : Bool = false | |||
There was a problem hiding this comment.
question: What's the purpose of using a class variable?
This property is entirely scoped to the .run method, so it looks like it could be a local variable there?
There was a problem hiding this comment.
Honestly didn't think it too much, I followed the pattern used for other options like --no-color, but thinking it now it definitely can be scoped just to .run.
I will change it later.
|
@straight-shoota is there other feedback or changes necessary? Would be possible for you to highlight any other needed adjustments (outside of the class_property change indicated above) so I can consolidate all my changes at once? Thank you. |
|
I don't have any further comments at this point. |
Revert the usage of class property introduced in 5a6dc6c and leverage instead on a pure instance variable in the context of `Shards.run`.
Hello, this PR aims to forward all arguments supplied to
shardsto the subcommand, if found.It does this to allow correct parsing of all ARGV supplied, including
SHARDS_OPTSthat might be appended, avoids altering the originalARGVwhen combining it.Introduces a naive integration test for subcommand to validate the change works correctly.
Last but not least, allows also forwarding
--helpby avoiding it to short-circuit and return immediately bysetting a flag for it and evaluating at the end of the processing of unknown options.
This is only done for the CLI invocation and is not part of Shards module (as the help and usage options are only available in this context).
I'm not sure about the implementation as this was the first integration test for subcommands, so would appreciate some notes on additional changes.
Thank you.
Fixes #600