Skip to content
This repository was archived by the owner on May 11, 2020. It is now read-only.

WIP - Import structopt #10

Closed
wants to merge 25 commits into from
Closed

WIP - Import structopt #10

wants to merge 25 commits into from

Conversation

kbknapp
Copy link
Member

@kbknapp kbknapp commented Nov 26, 2017

This is a work in progress, do not merge

Started doing the port of structopt and set up a bunch of initial scaffolding for this crate. Here's the key notes, which I'll add to as I make commits:

This branch is based on structopt@d983649822b

  • Code is set up as two crates
    • Mainline clap contains all the public derive traits and such (currently behind the unstable feature)
    • clap_derive (this crate) which contains the procedural macros and custom derive bits
  • The deriveable traits are:
    • IntoApp which provides:
      • into_app() -> clap::App
      • impl From<MyStruct> for clap::App
    • FromArgMatches which provides
      • from_argmatches(clap::ArgMatches) -> Self
      • try_from_argmatches(clap::ArgMatches) -> Result<Self, clap::Error>
      • impl From<clap::ArgMatches> for MyApp
      • once stable: impl TryFrom<clap::ArgMatches> for MyApp
    • Parse which provides
      • parse() -> Self (i.e. clap::App::get_matches)
      • try_parse() -> Result<Self, clap::Error> (i.e. clap::App::get_matches_safe)
      • parse_from() -> Self where I: IntoIterator<Item = T>, T: Into<OsString> (i.e. clap::App::get_matches_from)
      • try_parse_from(I) -> Result<Self, clap::Error> where I: IntoIterator<Item = T>, T: Into<OsString> (i.e. clap::App::get_matches_from_safe)
    • Clap which is just a convenience and provides all three of the above traits
    • ArgEnum which provides
      • variants() -> [&'static str; N]
      • impl FromStr for MyEnum
  • Code is dual licensed MIT/Apache 2.0
  • Integration tests are in clap_derive/tests
  • Examples will go in clap_derive/examples
  • Due to convention in the Rust Book, I renamed the repo and crate from clap-derives to clap_derive (no longer plural and uses snake case)
  • All version updates will be 0.x until this code compiles and works with clap 3.x at which point v3.0 of this repo will be released (i.e. major version will stay in sync)

Closes #8
Closes #5

This commit simply imports the structopt codebase and history and does some simple renames.

The codebase doesn't compile yet as this is purely a WIP and starting point.
kbknapp and others added 11 commits November 28, 2017 08:24
This commit starts to move the StructOpt trait into three smaller
traits (Parse, IntoApp, and FromArgMatches) as well as a convenience
triat which combines all three (Clap).

It also continues work on renaming structopt->clap

Finally, it starts to move all the impls into their own mods.
@kbknapp
Copy link
Member Author

kbknapp commented Jul 2, 2018

This is almost building. The primary issue is I can't seem to get the custom attributes to be recognized (#[clap(short = "f")] which makes me think it's because the traits have been divided out and all four are trying to claim the clap attribute?

Also, I've noticed a design flaw in splitting the traits; the Parse trait doesn't allow one to ergonomically say, let mystruct: MyStruct = MyStruct::parse() instead you have to do:

use clap::Parse;

let mystruct: MyStruct = <MyStruct as Parse>::parse();

which is silly. Instead I believe we should move the parse_* methods into the MyStruct impl which is built with the augmentation functions.

I've also removed the dep of clap proper and will be making clap_derive an optional dep of mainline shortly when I fix the trait design issue. I'll do some testing to see if my thoughts on the attributes issue is correct, and if so, will move all methods into a singular Clap trait and edit the top comment.

@TeXitoi
Copy link
Contributor

TeXitoi commented Jul 2, 2018

I can't see the trait definitions to understand your Parse problem.

That's a bit sad to loose all the structopt history and contributors. It would be great to be able to merge structopt in clap_derive during the transition. Spliting the trait can be done in StructOpt for testing, and moving the files of structopt for easy merging between StructOpt and clap_derive could be great.

I also thing that there is 2 breaking changes that can be interesting:

@kbknapp
Copy link
Member Author

kbknapp commented Jul 2, 2018

I can't see the trait definitions to understand your Parse problem.

They're defined in the clap repo, but look at the v3-master branch as that's the ones this crate is using. Based off my testing the attribute problem above, I may just keep one big trait like Structopt. I'll report back here with what I find and decide.

That's a bit sad to loose all the structopt history and contributors. It would be great to be able to merge structopt in clap_derive during the transition.

You're totally correct. Now that I'm getting more familiar with codebase I'm willing to scrap this PR and instead do a proper import. It's not my intention at all to remove the history or credit of those who have contributed!

I'd also be willing to delete this repo, and do all the work in the structopt proper repo. However, I'm not sure how you'd feel about renaming that repo and eventually transferring to the github.com/clap-rs org (which is where clap and all related crates will be moved).

The hardest part with working in the structopt proper repo is this code is setup like this:

  • structopt => clap proper (defines the Clap trait)
  • structopt-derive => clap_derive (the actual proc macros)

@kbknapp
Copy link
Member Author

kbknapp commented Jul 2, 2018

I commented on the first breaking change, but I'm less in favor of the second. I'm personally fine with the "forced" workaround listed, and not a huge fan of too much "black magic" like you mentioned 👍

@TeXitoi
Copy link
Contributor

TeXitoi commented Jul 2, 2018

So we agree on the features :-)

@kbknapp kbknapp closed this Jul 2, 2018
@kbknapp
Copy link
Member Author

kbknapp commented Jul 2, 2018

I've re-done the master branch of this repo to properly import structopt with all of it's history and commits. Some commits aren't pulled in (such as those on structopt/src/lib.rs) because it doesn't directly translate to something in this repo, as that logic is contained in the main clap repo.

Now I can start the work from this PR on the new master branch which will now keep track of the proper history.

@TeXitoi
Copy link
Contributor

TeXitoi commented Jul 2, 2018

I think the Parse trait is useless, just add the Parse method in FromArgMatches trait.

@TeXitoi
Copy link
Contributor

TeXitoi commented Jul 2, 2018

FromArgMatches::from_argmatches should be directly implemented in the trait using FromArgMatches::try_from_argmatches

@TeXitoi
Copy link
Contributor

TeXitoi commented Jul 2, 2018

And the Clap trait should implement useful method combining IntoClap and FromArgMatches as Clap::from_args

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

Successfully merging this pull request may close these issues.

3 participants