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

impl StructOpt for Box<impl StructOpt> #307

Merged
merged 1 commit into from Dec 21, 2019
Merged

Conversation

CreepySkeleton
Copy link
Collaborator

Closes #304

This PR makes is_subcommand, from_subcommand, and augment_clap part of the StructOpt trait (marking them as doc(hidden). I believe this is justifiable change because:

  • The change is 100% backward compatible.
  • These methods won't ever collide with user's methods.
  • This way we could add more impls in future with ease.
  • The change is fairly small and straightforward.

@TeXitoi
Copy link
Owner

TeXitoi commented Dec 14, 2019

The methods should just be declared, not implented with unimplemented!()

@CreepySkeleton
Copy link
Collaborator Author

The trick here is - what if someone wants not derive but implement StructOpt manually? A user would want to implement only clap and from_matches, they should know nothing about these hidden methods.

@TeXitoi
Copy link
Owner

TeXitoi commented Dec 16, 2019

But if they implement it manually, they expect it to work with generated ones, and that's not the case.

OTOH, adding new interface is a breaking change.

I don't know... I'll think a bit about it, will comeback later.

Copy link
Owner

@TeXitoi TeXitoi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you're OK with the drawbacks I've shown, I'm OK to merge this as is.

@CreepySkeleton
Copy link
Collaborator Author

Yes, I'm fine with that. It's hard to imagine someone out there was trying to implement StructOpt manually anyway :)

@CreepySkeleton CreepySkeleton merged commit 3534cb3 into master Dec 21, 2019
@CreepySkeleton CreepySkeleton deleted the boxed_types branch December 21, 2019 10:50
@TeXitoi
Copy link
Owner

TeXitoi commented Dec 22, 2019

v0.3.6 published

@bkchr
Copy link

bkchr commented Dec 23, 2019

We implemented StructOpt on our own ^^

@bkchr
Copy link

bkchr commented Dec 23, 2019

https://github.com/paritytech/substrate/blob/master/client/cli/src/params.rs#L957 for reference is someone is interested

@CreepySkeleton
Copy link
Collaborator Author

@bkchr OK, I have an idea on how to make this work without adding new methods to the StructOpt trait

@bkchr
Copy link

bkchr commented Dec 23, 2019

Don't worry. I just wanted to comment here. I already have a patch for us to fix the problem. I also find it much better to have the method defined in the trait :)

@bkchr
Copy link

bkchr commented Dec 23, 2019

Okay, you already yanked it ^^

@CreepySkeleton
Copy link
Collaborator Author

CreepySkeleton commented Dec 23, 2019

Okay, you already yanked it ^^

It was yanked for other reasons #315

@CreepySkeleton
Copy link
Collaborator Author

CreepySkeleton commented Dec 23, 2019

Also, @bkchr , why don't you just derive it there?

#[derive(Clone, Debug, Default, StructOpt)]
#[structopt(no_version)]
pub struct NoCustom {}

As far as I can see, it would generate the same code (I mean, the code working the same way) as you have written manually.

I hope you understand that augment_clap is entirely private API, it's subject to change without notice. Don't do that:

impl AugmentClap for NoCustom {
	fn augment_clap<'a, 'b>(app: App<'a, 'b>) -> App<'a, 'b> {
		app
	}
}

If you need to do something that's not covered by the public API, please, open an issue here and we will deal with that.

@bkchr
Copy link

bkchr commented Dec 23, 2019

Good question why I not used derive(StructOpt). I don't really remember. Regarding the augment_clap function, you are right. At the time I was writing this code, StructOpt seemed to sleep a little bit :)
I make myself some note to revisit the code and make an issue for the features we need.

Basically we want to support adding extensions to the cli interface, that the users can define.

@cecton
Copy link
Contributor

cecton commented Jan 14, 2020

[edit: deleted an created a separate issue]

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 this pull request may close these issues.

Subcommand configuration causes clippy::large_enum_variant to be emitted
4 participants