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

Triage Structopt PRs since the fork #2809

Closed
epage opened this issue Oct 4, 2021 Discussed in #2658 · 7 comments
Closed

Triage Structopt PRs since the fork #2809

epage opened this issue Oct 4, 2021 Discussed in #2658 · 7 comments
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies C-enhancement Category: Raise on the bar on expectations
Milestone

Comments

@epage
Copy link
Member

epage commented Oct 4, 2021

Discussed in #2658

Originally posted by epage August 2, 2021
Dependabot notified me of a structopt update that included TeXitoi/structopt#483. This seems relevant for clap but made me wonder about the problem generally.

When was our last sync-up with structopt and how do we track it so things don't fall through the cracks for future changes?

@epage epage added C-bug Category: Updating dependencies C-enhancement Category: Raise on the bar on expectations D: medium A-derive Area: #[derive]` macro API labels Oct 4, 2021
@epage epage added this to the 3.0 milestone Oct 4, 2021
@epage
Copy link
Member Author

epage commented Oct 4, 2021

ea76fa1 is the last commit in https://github.com/clap-rs/clap_derive/commits/master?after=acea444f962a0d7c8126273739d4df8bdb152624+34&branch=master from purely structopt. It looks like some fixes went in since then but unsure which

Post fork, already ported

Start of new port series

@epage
Copy link
Member Author

epage commented Oct 4, 2021

@pksunkara and @CreepySkeleton since you two were involved a lot during the late stages of clap_derive, if its quick to do a scan to see what is already ported to clap_derive, that'd be appreciated. If not, I'm fully willing to take on doing this deep dive.

@epage
Copy link
Member Author

epage commented Oct 5, 2021

Looks like I found the point of divergence. We have everything up to 401 and need 414 and after

epage added a commit to epage/clap that referenced this issue Oct 6, 2021
TeXitoi/structopt#414 was ported in
a951958 but the test was less exhaustive.  This updates our test to
match structopt's latest version of the test.

This is a part of clap-rs#2809
epage added a commit to epage/clap that referenced this issue Oct 6, 2021
Some programs do not use anything to separate word boundaries.

For example a struct may contain the field `build_dir` while the flag is
`--builddir`.

This is a port of TeXitoi/structopt#412

This is part of clap-rs#2809
epage added a commit to epage/clap that referenced this issue Oct 6, 2021
This carries over a test case from
TeXitoi/structopt#448, and re-fixes it according
to the changes we've made since we forked.  I also tried to  identify
other cases and quote them to avoid playing whack-a-mole with this.

This is a part of clap-rs#2809
epage added a commit to epage/clap that referenced this issue Oct 6, 2021
epage added a commit to epage/clap that referenced this issue Oct 6, 2021
…us type of `map`

> This PR closes clap-rs#490. Please refer to clap-rs#490 for the detail of the problem. Let me know if you want to make `convert_type` a function.

This is a port of TeXitoi/structopt#491

This is part of clap-rs#2809
epage added a commit to epage/clap that referenced this issue Oct 6, 2021
epage added a commit to epage/clap that referenced this issue Oct 7, 2021
This carries over a test case from
TeXitoi/structopt#448, and re-fixes it according
to the changes we've made since we forked.  I also tried to  identify
other cases and quote them to avoid playing whack-a-mole with this.

This is a part of clap-rs#2809
epage added a commit to epage/clap that referenced this issue Oct 7, 2021
epage added a commit to epage/clap that referenced this issue Oct 7, 2021
Some programs do not use anything to separate word boundaries.

For example a struct may contain the field `build_dir` while the flag is
`--builddir`.

This is a port of TeXitoi/structopt#412

This is part of clap-rs#2809
epage added a commit to epage/clap that referenced this issue Oct 7, 2021
This carries over a test case from
TeXitoi/structopt#448, and re-fixes it according
to the changes we've made since we forked.  I also tried to  identify
other cases and quote them to avoid playing whack-a-mole with this.

This is a part of clap-rs#2809
epage added a commit to epage/clap that referenced this issue Oct 7, 2021
TeXitoi/structopt#414 was ported in
a951958 but the test was less exhaustive.  This updates our test to
match structopt's latest version of the test.

This is a part of clap-rs#2809
@pksunkara
Copy link
Member

  • No coverage for test

@epage
Copy link
Member Author

epage commented Oct 11, 2021

* [ ]  No coverage for [test](https://github.com/TeXitoi/structopt/pull/401/files#diff-57e8e1f836204e3d43fdd26c8c05d8aa25edc6db8c921f99f5d482c3f56e05a9R134)

I'm not even sure what that is trying to test; I forgot what structopt does when flattening in that context.

As for clap_derive, we don't even allow an enum to flatten a struct, its a compiler error due to mismatched types. Of the various combinations of bad flatten and subcommand, it looks like the only one we cover is

#[derive(clap::Parser)]
struct Opt {
    #[clap(flatten)]
    sub: SubCmd,
}

#[derive(clap::Parser)]
enum SubCmd {}

fn main() {}

I'm mixed on the value of expanding out the rest of the combinations but either way, don't think it would block 3.0.

@epage
Copy link
Member Author

epage commented Oct 11, 2021

I'm delegating the generics work to #2769 which means we are done or practically done with this. Rather than this lingering on for each new issue, I'm going to go ahead and close it. We can create new issues for each additional structopt porting effort and prioritize accordingly.

@epage epage closed this as completed Oct 11, 2021
@pksunkara
Copy link
Member

As for clap_derive, we don't even allow an enum to flatten a struct, its a compiler error due to mismatched types.

Yeah, we need an UI test for that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-derive Area: #[derive]` macro API C-bug Category: Updating dependencies C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants