Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Forward unmodified ARGV to subcommand #631

Merged

Conversation

luislavena
Copy link
Contributor

Hello, this PR aims to forward all arguments supplied to shards to the subcommand, if found.

It does this to allow correct parsing of all ARGV supplied, including SHARDS_OPTS that might be appended, avoids altering the original ARGV when combining it.

Introduces a naive integration test for subcommand to validate the change works correctly.

Last but not least, allows also forwarding --help by avoiding it to short-circuit 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'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

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).
@ysbaddaden
Copy link
Contributor

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:

  • shards --help dummy would let shards handle --help
  • shards dummy --help would call shards-dummy --help

@luislavena
Copy link
Contributor Author

@ysbaddaden I like that too, even it mentions git help -a to list all the available subcommands since the default --help lists only some of the core (porcelain) ones (like init, merge, commit, branch, etc).

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 --help, that way might avoid confusion.

Open to suggestions!

Thank you for looking into it!
❤️ ❤️ ❤️

spec/integration/subcommand_spec.cr Outdated Show resolved Hide resolved
src/cli.cr Show resolved Hide resolved
Follow the pattern used in other tests and use simple `sh` script on
non-Windows platforms.
src/cli.cr Outdated Show resolved Hide resolved
@luislavena
Copy link
Contributor Author

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

@straight-shoota
Copy link
Member

I don't have any further comments at this point.
You could wait for feedback from others, if you want.

Revert the usage of class property introduced in
5a6dc6c and leverage instead on a pure
instance variable in the context of `Shards.run`.
src/cli.cr Show resolved Hide resolved
@straight-shoota straight-shoota added this to the 0.19.0 milestone Aug 13, 2024
@straight-shoota straight-shoota merged commit 48b7295 into crystal-lang:master Aug 15, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Certain CLI options are not passed to shards custom subcommands
4 participants