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

Refactor clap_generate #1678

Merged
merged 3 commits into from
Feb 8, 2020
Merged

Refactor clap_generate #1678

merged 3 commits into from
Feb 8, 2020

Conversation

pksunkara
Copy link
Member

@pksunkara pksunkara commented Feb 5, 2020

I have copied the code from clap_generate and refactored the structure a bit.

This new structure will allow people to write their own generators using our Generator trait which will contain some helpers (Still working on polishing them).

@pksunkara pksunkara force-pushed the generate branch 4 times, most recently from d0b92dd to 3b8527f Compare February 5, 2020 22:20
@pksunkara
Copy link
Member Author

The tests are failing because something changed in clap v3 since the subcommand -h and subcommand -V are not getting printed in completions

@CreepySkeleton
Copy link
Contributor

I would rather postpone any work on clap_generate for the time being. let's focus our efforts on clap_derive first, then on the next alpha release, and then we can move to other stuff.

@pksunkara
Copy link
Member Author

I thought it would be just moving it to this repo, but then I got bogged down with refactoring. 😞

@CreepySkeleton
Copy link
Contributor

CreepySkeleton commented Feb 6, 2020 via email

@pksunkara
Copy link
Member Author

I haven't pulled this PR because man is going to be changed according to recent discussions. Other than that, this PR is good to go and this crate should be good to release when we are doing 3.0

@pksunkara pksunkara marked this pull request as ready for review February 6, 2020 22:57
@pksunkara pksunkara force-pushed the generate branch 3 times, most recently from 72ea56b to 2720413 Compare February 7, 2020 06:20
@pksunkara
Copy link
Member Author

I assume you gave a close look at this PR because there probably will be no changes for 3.0 in this crate.

@CreepySkeleton
Copy link
Contributor

Not so close, just reviewed the public API and it looks pretty good. I haven't looked at the impl.

@pksunkara
Copy link
Member Author

Just validate the test/completions.rs from this and clap_generate repo. The fixtures should be the same.

@CreepySkeleton
Copy link
Contributor

There's no test/completions.rs in https://github.com/clap-rs/clap/tree/generate/tests 😕

@pksunkara
Copy link
Member Author

@CreepySkeleton
Copy link
Contributor

Looks fine.

@pksunkara
Copy link
Member Author

bors r=CreepySkeleton

bors bot added a commit that referenced this pull request Feb 8, 2020
1678: Refactor clap_generate r=CreepySkeleton a=pksunkara

I have copied the code from [clap_generate]( https://github.com/clap-rs/clap_generate) and refactored the structure a bit.

This new structure will allow people to write their own generators using our `Generator` trait which will contain some helpers (Still working on polishing them).

Co-authored-by: Ole Martin Ruud <[email protected]>
Co-authored-by: Pavan Kumar Sunkara <[email protected]>
edition = "2018"
authors = [
"Kevin K. <[email protected]>",
"Pavan Kumar Sunkara <[email protected]>",

Choose a reason for hiding this comment

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

We exist as well :p

Copy link
Member Author

Choose a reason for hiding this comment

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

I have been meaning to just remove all the names and put "Clap Contributors". What do you think?

Choose a reason for hiding this comment

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

That was going to be my next suggestion. The Clap Team or Clap Maintainers

Copy link
Contributor

Choose a reason for hiding this comment

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

I would like to propose leave @kbknapp - as original author of these crates - and @TeXitoi (in clap_derive/Cargo.toml) - as original author of the derive; and then put "Clap Contributors". I want to highlight them because they have (or had) been putting admirable amount of efforts into these crates from the very beginning, even thought the crates weren't so hypy and popular back then.

Choose a reason for hiding this comment

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

That sounds fine to me as well. I'd rather go with Maintainers though than contributors. There's a subtle difference

use std::io::Write;

// Internal
use clap::*;

Choose a reason for hiding this comment

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

Import only what we require not the entire set

Copy link
Member Author

Choose a reason for hiding this comment

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

Will fix in a new PR.

@bors
Copy link
Contributor

bors bot commented Feb 8, 2020

Build succeeded

@bors bors bot merged commit da7e9e5 into master Feb 8, 2020
@bors bors bot deleted the generate branch February 8, 2020 16:01
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.

4 participants