WIP: Extract subcommands into separate trait #1681

CreepySkeleton · 2020-02-08T17:10:13Z

Not-yet-working-but-almost-there "multiple traits" approach. More or less done, what's left is to catch some bugs and adapt tests/examples.

For the record: it took so long because of RL stuff (who would have thought?) and because [there was a detailed description of the experience I've had here, but it was deleted because it contained a lot of profanity and emotional notes].

As the only person alive that understands how the derive works (if you won't blow your own horn, nobody will do it for you, yeah), I'd like to made a statement: we Do need the refactoring.

pksunkara · 2020-02-08T19:30:18Z

Conflicts? And yeah, I think it will take me a bit of time to check this.

I don't know about the other traits we were talking about, but I was pretty sure we didn't need subcommand trait.

TeXitoi

That's a bit long, I'll try to read it layer.

clap_derive/examples/enum_tuple.rs

CreepySkeleton · 2020-02-08T19:50:56Z

I've splited the PR into separate commits so it's hopefully easier to review :)

A separate trait for subcommands was the whole point. It allows us to check that #[clap(subcommand)] is applied to enum and not to struct. It also moves unrelated functions into separate, unrelated traits, which is easier to maintain than one giant trait. I was also thinking about Flattened trait but figured that intoApp will depen on it one way or another and so the trait will be virtually useless.

CreepySkeleton · 2020-02-08T19:57:02Z

clap_derive/tests/subcommands.rs

@@ -162,13 +162,14 @@ fn test_tuple_commands() {
 }

 #[test]
+#[ignore] // FIXME (@CreepySkeleton)


This is not working yet. I know what this is all about, I know how to fix it, I'll get to it tomorrow.

Upon further review, I'm totally convinced that this is an error in the test, see my comment below.

CreepySkeleton · 2020-02-08T19:57:35Z

clap_derive/tests/subcommands.rs

@@ -185,13 +186,14 @@ fn enum_in_enum_subsubcommand() {
 }

 #[test]
+#[ignore]


Not working, I'm aware, will fix

It tests an undocumented feature and I highly suspect that it wasn't intended to work that way, see my comment below.

src/lib.rs

pksunkara

I am still reviewing the main code changes. But basically, the Subcommand introduced in the following commits should be removed from the PR.

5eb8551
5eb8551

And 6184733 should be removed completely

clap_derive/examples/enum_tuple.rs

src/lib.rs

CreepySkeleton · 2020-02-11T20:40:04Z

People, I think I've found a bug in the derive which has become yet another example of "this is not a bug anymore, but a feature". Two related bugs, actually. The is_subcommand method exists solely to support this bugs.

Consider this code:

#[derive(Clap)]
struct Foo {
    // we should have used `subcommand` here
    // this is an error
    #[clap(flatten)]
    sub: Sub
}

#[derive(Subcommand)]
enum Sub { /* whatev */ }

Do you think it would fail to compile? Or, maybe panic with unwrapped None? Hell no! It works perfectly, effectively making flatten alias to subcommand. There's no mentioning in strustopt docs about it. @TeXitoi did you intend it to work this way?

The second problem is more subtle.

enum Foo {
    // this is supposed to work as `flatten` 
    // https://docs.rs/structopt/0.3.9/structopt/#subcommands
    Foo(Ext)
}

// This is supposed to be a struct
enum Ext {}

This is how the snippet was supposed to be written, according to the docs:

enum Foo {
    // flattening struct, all good
    Foo(Ext)
}

struct Ext {
    #[clap(subcommand)]
    sub: ExtSub
}

enum ExtSub {}

But it works, miraculously. structopt tries to "figure out what's on the other side" via is_subcommand.

Instead of these "magical" solutions, I propose the alternative

enum Foo {
    // no attr = flatten, backcompat
    Foo(Ext),
   
    // tell derive that ExtFlat is struct and it must be flattened 
   #[clap(flatten)]
   Bar(ExtFlat),

   // tell derive that ExtSub is enum and it must be used as subcommands set
   #[clap(subcommand)]
   Bar(ExtSub),

   // synonym for current #[structopt(flatten)]
   // see https://github.com/TeXitoi/structopt/issues/327
   #[clap(subcommand(flatten))]
   Zog(ExtSubFlat)
}

CreepySkeleton · 2020-02-11T20:42:16Z

I don't like this commit. We should just have the users do derive(Clap) and our library should be smart to assume Subcommand since it's an enum.

Could you please elaborate on that? I've got a strong feeling we're talking about different things here.

pksunkara · 2020-02-11T20:45:03Z

Could you please elaborate on that? I've got a strong feeling we're talking about different things here.

It is related to #1681 (comment)

CreepySkeleton · 2020-02-11T20:52:56Z

@pksunkara Let's make sure we're talking about the same thing.

trait Clap is a set of functions that work on top of IntoApp + FromArgMatches. Users should derive it for an enum only if they want to use something like

#[derive(Clap)]
enum Foo { }

let args = Foo::parse();

Subcommand is to support #[clap(subcommand)]. Users use it if they only want the enum to be #[clap(subcommand)]ed.

You're proposing to make #[derive(Clap)] on enum also implement Subcommand. Is that right?

pksunkara · 2020-02-11T20:59:54Z

Yes. What I am trying to aim for here is for the user to just simply do derive(Clap) and we handle everything for him.

pksunkara

So, I finally finished the review. Nothing wrong with the code much. I am just thinking about the API.

Can't we have #[clap(into_app)] special for enums that want to ::parse()? Then, we can simply do #[derive(Clap)] everywhere

clap_derive/src/lib.rs

clap_derive/src/derives/attrs.rs

clap_derive/src/derives/subcommand.rs

pksunkara

I still see #[derive(Subcommand)] in the examples and tests, is that intentional?

clap_derive/examples/rename_all.rs

clap_derive/src/derives/clap.rs

pksunkara

Just minor changes and I can merge this.

clap_derive/tests/issues.rs

pksunkara · 2020-02-12T19:47:07Z

clap_derive/tests/subcommands.rs

@@ -161,47 +161,52 @@ fn test_tuple_commands() {
    assert!(!output.contains("Not shown"));
 }

-#[test]


Let's create an issue to rethink about Flatten and Composability.

Mind explaining what you mean by "Composability"?

I am thinking of the future. I actually started looking into Clap because I was working on a higher level CLI framework. In this, I wanted people to make plugins and CLI to use them. For example, I wanted to provide a plugin which provides a group of subcommands that I want a CLI to have at top-level, I would want to use flatten. But this is not the only scenario.

What I am saying is we want to make sure we support all kinds of things people want to do if they can share Args and Apps.

Dylan-DPC-zz · 2020-02-12T20:39:01Z

Let's discuss about the future later :p

pksunkara

bors r+

1681: WIP: Extract subcommands into separate trait r=pksunkara a=CreepySkeleton Not-yet-working-but-almost-there "multiple traits" approach. More or less done, what's left is to catch some bugs and adapt tests/examples. For the record: it took so long because of RL stuff (who would have thought?) and because [there was a detailed description of the experience I've had here, but it was deleted because it contained a lot of profanity and emotional notes]. As the only person alive that understands how the derive works (if you won't blow your own horn, nobody will do it for you, yeah), I'd like to made a statement: we Do need the refactoring. Co-authored-by: CreepySkeleton <[email protected]>

bors · 2020-02-12T21:03:17Z

Build succeeded

clap-rs.clap
continuous-integration/travis-ci/push

`structopt` originally allowed ``` pub enum Opt { Daemon(DaemonCommand), } pub enum DaemonCommand { Start, Stop, } ``` This was partially broken in clap-rs#1681 where `$ cmd daemon start` works but `cmd daemon`, panics. Originally, `structopt` relied on exposing the implementation details of a derived type by providing a `is_subcommand` option, so we'd know whether to provide `SubcommandRequiredElseHelp` or not. This was removed in clap-rs#1681 Fixes clap-rs#2005

CreepySkeleton changed the title ~~Extract subcommands into separate trait~~ WIP: Extract subcommands into separate trait Feb 8, 2020

pksunkara self-requested a review February 8, 2020 19:30

CreepySkeleton force-pushed the multiple-traits branch from 802ba1e to 6dc44fd Compare February 8, 2020 19:42

CreepySkeleton requested review from Dylan-DPC-zz and TeXitoi February 8, 2020 19:43

TeXitoi reviewed Feb 8, 2020

View reviewed changes

clap_derive/examples/enum_tuple.rs Outdated Show resolved Hide resolved

CreepySkeleton commented Feb 8, 2020

View reviewed changes

pksunkara reviewed Feb 11, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

pksunkara requested changes Feb 11, 2020

View reviewed changes

clap_derive/examples/enum_tuple.rs Outdated Show resolved Hide resolved

src/lib.rs Outdated Show resolved Hide resolved

pksunkara requested changes Feb 12, 2020

View reviewed changes

clap_derive/src/lib.rs Outdated Show resolved Hide resolved

clap_derive/src/derives/attrs.rs Show resolved Hide resolved

clap_derive/src/derives/subcommand.rs Outdated Show resolved Hide resolved

CreepySkeleton force-pushed the multiple-traits branch 3 times, most recently from 2803fe5 to 17f9eca Compare February 12, 2020 14:55

pksunkara requested changes Feb 12, 2020

View reviewed changes

clap_derive/examples/rename_all.rs Outdated Show resolved Hide resolved

clap_derive/src/derives/clap.rs Outdated Show resolved Hide resolved

CreepySkeleton added the R: work in progress label Feb 12, 2020

pksunkara requested changes Feb 12, 2020

View reviewed changes

Extract subcommands into separate trait

ae574df

CreepySkeleton force-pushed the multiple-traits branch from 690482d to ae574df Compare February 12, 2020 20:15

pksunkara approved these changes Feb 12, 2020

View reviewed changes

pksunkara removed the R: work in progress label Feb 12, 2020

bors bot merged commit ae574df into master Feb 12, 2020

bors bot deleted the multiple-traits branch February 12, 2020 21:03

CreepySkeleton mentioned this pull request Feb 14, 2020

flatten on enums does not trigger error, while it should TeXitoi/structopt#328

Closed

CreepySkeleton mentioned this pull request Jul 9, 2020

Use enum as subcommands in a subcommand #2005

Closed

2 tasks

epage mentioned this pull request Jul 14, 2021

fix(derive)!: Compile-error on nested subcommands #2586

Merged

epage mentioned this pull request Jul 14, 2021

fix(derive): Allow subcommands to directly nest in subcommands #2587

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Extract subcommands into separate trait #1681

WIP: Extract subcommands into separate trait #1681

CreepySkeleton commented Feb 8, 2020

pksunkara commented Feb 8, 2020

TeXitoi left a comment

CreepySkeleton commented Feb 8, 2020 •

edited

Loading

CreepySkeleton Feb 8, 2020

CreepySkeleton Feb 11, 2020

CreepySkeleton Feb 8, 2020

CreepySkeleton Feb 11, 2020

pksunkara left a comment

CreepySkeleton commented Feb 11, 2020 •

edited

Loading

CreepySkeleton commented Feb 11, 2020

pksunkara commented Feb 11, 2020

CreepySkeleton commented Feb 11, 2020 •

edited

Loading

pksunkara commented Feb 11, 2020

pksunkara left a comment

pksunkara left a comment

pksunkara left a comment

pksunkara Feb 12, 2020

CreepySkeleton Feb 12, 2020

pksunkara Feb 12, 2020

Dylan-DPC-zz commented Feb 12, 2020

pksunkara left a comment

bors bot commented Feb 12, 2020

WIP: Extract subcommands into separate trait #1681

WIP: Extract subcommands into separate trait #1681

Conversation

CreepySkeleton commented Feb 8, 2020

pksunkara commented Feb 8, 2020

TeXitoi left a comment

Choose a reason for hiding this comment

CreepySkeleton commented Feb 8, 2020 • edited Loading

CreepySkeleton Feb 8, 2020

Choose a reason for hiding this comment

CreepySkeleton Feb 11, 2020

Choose a reason for hiding this comment

CreepySkeleton Feb 8, 2020

Choose a reason for hiding this comment

CreepySkeleton Feb 11, 2020

Choose a reason for hiding this comment

pksunkara left a comment

Choose a reason for hiding this comment

CreepySkeleton commented Feb 11, 2020 • edited Loading

CreepySkeleton commented Feb 11, 2020

pksunkara commented Feb 11, 2020

CreepySkeleton commented Feb 11, 2020 • edited Loading

pksunkara commented Feb 11, 2020

pksunkara left a comment

Choose a reason for hiding this comment

pksunkara left a comment

Choose a reason for hiding this comment

pksunkara left a comment

Choose a reason for hiding this comment

pksunkara Feb 12, 2020

Choose a reason for hiding this comment

CreepySkeleton Feb 12, 2020

Choose a reason for hiding this comment

pksunkara Feb 12, 2020

Choose a reason for hiding this comment

Dylan-DPC-zz commented Feb 12, 2020

pksunkara left a comment

Choose a reason for hiding this comment

bors bot commented Feb 12, 2020

Build succeeded

CreepySkeleton commented Feb 8, 2020 •

edited

Loading

CreepySkeleton commented Feb 11, 2020 •

edited

Loading

CreepySkeleton commented Feb 11, 2020 •

edited

Loading