fix: --verbose option not working for run-ios
#1571
Conversation
Caused by the fact that the root command has a verbose option too. This 'steals' it from the the run-ios sub-command in commander 2.x.
|
There hasn't been any activity on this pull request in the past 3 months, so it has been marked as stale and it will be closed automatically if no further activity occurs in the next 7 days. |
|
Still needs approval and merging |
| @@ -137,6 +137,13 @@ function attachCommand<IsDetached extends boolean>( | |||
| const passedOptions = this.opts(); | |||
| const argv = Array.from(args).slice(0, -1); | |||
|
|
|||
| // Copy parent commander verbose value to the sub-command if the sub-command also has a verbose option | |||
| // This is a workaround that can potentially be removed if commander is upgraded to v7+ | |||
There was a problem hiding this comment.
commander is now at v9. Is this fix relevant then? If so, please update the comment
There was a problem hiding this comment.
Oh, I wasn't aware commander had been upgraded, that's good! 😄 That does potentially change things a bit.
How that affects the direction of the PR will depend a bit on how the core maintainers want things to work overall in the CLI. Some of the routes now open to us would affect option parsing across all commands.
Firstly, I believe the current workarounds would still work in the new version of commander but I've not tested them yet.
As for alternatives, there are multiple option parsing methods in the newer version of commander which are potentially relevant (full docs w/ examples);
.enablePositionalOptions()
With positional options, the -b is a program option in the first line and a subcommand option in the second line:
program -b subcommand program subcommand -b
So, if this was enabled react-native --verbose run-ios would be different to react-native run-ios --verbose. I guess you could also specify react-native --verbose run-ios --verbose to up the verbosity at both levels?
.passThroughOptions()
By default options are recognised before and after command-arguments. To only process options that come before the command-arguments, use .passThroughOptions(). This lets you pass the arguments and following options through to another program without needing to use -- to end the option processing. To use pass through options in a subcommand, the program needs to enable positional options.
With pass through options, the
--port=80is a program option in the first line and passed through as a command-argument in the second line:program --port=80 arg program arg --port=80By default the option processing shows an error for an unknown option. To have an unknown option treated as an ordinary command-argument and continue looking for options, use
.allowUnknownOption(). This lets you mix known and unknown options.By default the argument processing does not display an error for more command-arguments than expected. To display an error for excess arguments, use
.allowExcessArguments(false).
This one might also be relevant but I'm not sure how it would behave in the CLI until I have a chance to play with it (due to the dynamic attachCommand setup).
Also, in this specific situation with --verbose I don't yet know if the recognised options (e.g. top level --verbose) still get hidden from the pass-through.
Is there a preferred way the core maintainers would like the options to behave in the CLI? If so I can look at changing the PR.
There was a problem hiding this comment.
So I think we can use enablePositionalOptions(), this fixes this issue and we don't need to make workarounds.
|
Any thoughts on which way you'd like me to go with the commander changes? (see review comment discussion above) |
|
@liamjones Would you like to rebase and update the code with latest comments, and get this merged? 😄 |
|
@szymonrybczak Sure, I'm just trying to find the time to work on it again 😅 |
|
Closing since #1946 was merged |
Fixes #1391
Summary:
This commit implements a workaround to get the verbose option working again on run-ios. It not working is caused by the fact that the root command has a verbose option too. This 'steals' it from the the run-ios sub-command in commander 2.x. See #1391 (comment) for more information.
Test Plan:
Added unit tests for the new code. Testing
rundirectly looked like it was going to be a hassle so I exportedattachCommandso I could test it directly, I hope that's okay.