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

Subcommand configuration causes clippy::large_enum_variant to be emitted #304

Closed
vmalloc opened this issue Dec 7, 2019 · 11 comments · Fixed by #307
Closed

Subcommand configuration causes clippy::large_enum_variant to be emitted #304

vmalloc opened this issue Dec 7, 2019 · 11 comments · Fixed by #307

Comments

@vmalloc
Copy link
Contributor

vmalloc commented Dec 7, 2019

I'm using the (very awesome and useful, by the way) pattern of defining subcommands with specific configurations:

enum Command {
    #[structopt(name = "version")]
    PrintVersion,

    #[structopt(name = "second")]
    DoSomething {
        #[structopt(flatten)]
        config: DoSomethingConfig,
    },

    #[structopt(name = "first")]
    DoSomethingElse {
        #[structopt(flatten)]
        config: DoSomethingElseConfig,
    }
...
}

The problem is that with my current project, consisting of about 5 subcommands with varying configuration complexities, clippy emits the clippy::large_enum_variant warning.

I'm fine with silencing the warning since it's a single point which is not mission-critical at all (parsing command-line args). However it got me wondering if it would be possible to use Boxes to contain the flattened configurations as suggested by the lint... Sounds to me like something that could be supported with Structopt's trait implementation?

Sorry in advance if this is spam, and thanks for this amazing project!

@CreepySkeleton
Copy link
Collaborator

CreepySkeleton commented Dec 7, 2019

Sorry in advance if this is spam, and thanks for this amazing project!

You can rest assured that questions, requests for a feature, bug reports and the like won't ever be considered spam here. And you're welcome!

As regards to your suggestion for boxed variants... well we could add impl<T> StructOpt for Box<T> where T: StructOpt. That would allow using boxes directly in places where StructOpted types are expected, like flatten or subcommand

enum Command {
    #[structopt(name = "version")]
    PrintVersion,

    #[structopt(name = "second")]
    DoSomething {
        #[structopt(flatten)]
        config: Box<DoSomethingConfig>,
    },

    #[structopt(name = "first")]
    DoSomethingElse {
        #[structopt(flatten)]
        config: Box<DoSomethingElseConfig>,
    }
}

as long as the underlying (boxed) type implements StructOpt.

@TeXitoi what do think about it?

@vmalloc
Copy link
Contributor Author

vmalloc commented Dec 7, 2019

@CreepySkeleton I really appreciate that! Very glad to see that this mentality is in place here, unlike other projects that shall go unnamed ;-)

If this is fine with you and @TeXitoi, I can try and craft a PR to add this

@CreepySkeleton
Copy link
Collaborator

CreepySkeleton commented Dec 7, 2019

I really appreciate that! Very glad to see that this mentality is in place here, unlike other projects that shall go unnamed ;-)

It doesn't really mean those unnamed maintainers are bad or lazy. Maintaining takes time and requires considerable effort put in; the more features a project offers the harder it is to maintain, especially with backward compatibility in mind. Some people don't have much time to spare :(

Also, we don't really promise that every single feature requested will be implemented, see #128 for example.

P.S. Why do you implement "print version" as a subcommand? It looks like virtually every app out there does this via short or long flags, like -v or --version.

@vmalloc
Copy link
Contributor Author

vmalloc commented Dec 7, 2019

That's just an example, not a real use case :-)

As for maintaining projects go, there's a difference between saying "I won't get to it" and just closing issue without commenting on them, or while adding snarky/berating responses to the people who opened them. I'm a maintainer myself and try to extend at least the minimal amount of patience towards people who ask questions or request features that already exist...

@CreepySkeleton
Copy link
Collaborator

cc @TeXitoi this is really easy, literally ~10 more lines in src/lib.rs

@TeXitoi
Copy link
Owner

TeXitoi commented Dec 11, 2019

If that's only an impl on the structopt crate, OK, but I suspect it will be more complicated with the augment hidden methods.

@vmalloc
Copy link
Contributor Author

vmalloc commented Dec 14, 2019

@TeXitoi @CreepySkeleton Are there any special instructions on how to run the tests apart from cargo test --all-features? I'm trying it on stable but getting:

error[E0463]: can't find crate for `clippy`
   --> /Users/rotemy/.cargo/registry/src/github.com-1ecc6299db9ec823/clap-2.33.0/src/lib.rs:528:39
    |
528 | #![cfg_attr(feature = "lints", plugin(clippy))]
    |                                       ^^^^^^ can't find crate

I think there should be some clause in the README on how to build or test it for submitting contributions...

@TeXitoi
Copy link
Owner

TeXitoi commented Dec 14, 2019

Just cargo test --all as you can see on the .travis.yaml

@vmalloc
Copy link
Contributor Author

vmalloc commented Dec 14, 2019

Well, .travis.yaml shows:

cargo test $FEATURES

and I am guessing FEATURES is defined in Travis' config somewhere because I can't grep for it in the code...

@TeXitoi
Copy link
Owner

TeXitoi commented Dec 14, 2019

There was a nightly feature before that was tested on Travis. Removed now, bit we must forgot to remove that.

@CreepySkeleton
Copy link
Collaborator

@vmalloc and this is problem in clap v2.33 anyway 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants