External subcommand #1883
External subcommand #1883
Conversation
| } | ||
| }); | ||
| }) | ||
| .collect(); |
There was a problem hiding this comment.
I don't think you need the collect here.
There was a problem hiding this comment.
I need collect here so this mapping is executed right now, not lazily. I need it because it sets ext_subcmd and the code below relies on it.
| other => { | ||
| #( #child_subcommands )*; | ||
| None | ||
| #( #child_subcommands )else* |
There was a problem hiding this comment.
I don't understand the else here.
There was a problem hiding this comment.
This else serves as a separator for if statements.
Before:
if let Some(...) = ... {
//...
}
if let Some(...) = ... {
//...
}
// ...After:
if let Some(...) = ... {
//...
} else if let Some(...) = ... {
//...
}
// ...The new variant looks better in cargo expand, this is the whole motivation
|
Looks good except the 2 comments. |
CreepySkeleton
force-pushed
the
external_subcommand
branch
from
f26a98c
to
7c45591
Compare
|
You are not running tests locally. They will fail with this commit. |
|
@pksunkara Why do you think so? Clarify? |
|
You uncommented the asserts in the test as I asked, but you forgot to uncomment the enum variants which are used in the asserts. |
CreepySkeleton
force-pushed
the
external_subcommand
branch
from
7c45591
to
6bdb6d9
Compare
|
Yeah, fixed. I tend to avoid running tests on my machine because CI works 2x faster. |
|
Rust Analyzer with VSCode allows running single test easily |
|
...except my current laptop is slow as hell. Let's try to integrate the current workflow - I push, look at the logs, fix, push, request review when it's green. |
|
I do run single tests, but sometimes it's not enough. |
|
@CreepySkeleton @pksunkara see the discussion in the Admins area of the org if you'd like to offload some compile/test times without having to use the public CI/PR trackers. |
|
Okay, forget the above comments. Let me rephrase the whole thing. I am missing UI tests for the following:
|
|
I'm going to reorganize the |
|
Can we please not do the reorganization as part of this PR? |
|
bors r+ |
|
Build succeeded: |
Closes #1672
This is almost done, but blocked on the undetected bug in 3.0: if both
AllowExternalSubcommandsandSubcommandRequiredElseHelpare set, externals subcommand doesn't count and exit triggers. Will get to it tomorrow.