fix: Ease clap2->clap3 migration with deprecations #2718

epage · 2021-08-18T15:53:53Z

This was done based on the README. I've not done a deep dive of the docs to see what might have been missed.

App::with_defaults was not included since that has been deprecated
since 2.14
App::args_from_usage does not have a close enough parallel in the
new API, as far as I could tell
ArgMatches::usage cannot have a thin wrapper around
App::generate_usage.
App::write_*: getting lazy, didn't seem like high value functions
Any Settings (some things need to be figured out here)

This is a part of #2617

epage · 2021-09-08T18:30:12Z

@pksunkara any update on this? It passes CI, just re-pushed as part of rebase

pksunkara · 2021-10-01T00:24:54Z

I am waiting on this because we have to finalize the discussion on #2617

epage · 2021-10-12T13:30:33Z

Looks like we agreed in #2617 on this direction. Going ahead with a post-merge review.

bors r+

2718: fix: Ease clap2->clap3 migration with deprecations r=epage a=epage Co-authored-by: Ed Page <eopage@gmail.com>

pksunkara · 2021-10-12T13:32:04Z

I don't think we agreed on there, right? At least that's the impression I get. Can we please wait on this a bit more? There is no need for urgency

epage · 2021-10-12T13:32:40Z

bors r-

bors · 2021-10-12T13:32:41Z

Canceled.

epage · 2021-10-12T13:35:56Z

I don't think we agreed on there, right? At least that's the impression I get. Can we please wait on this a bit more?

@kbknapp seemed in favor of this 10 days ago and I hadn't seen a post from you.

There is no need for urgency

This has been open for 2 months. I have to deal with merge conflicts, designs are blocked on a decision, and mental state gets lost, like with #2721. We can't just let these things be left around.

- `App::with_defaults` was not included since that has been deprecated since 2.14 - `App::args_from_usage` does not have a close enough parallel in the new API, as far as I could tell - `ArgMatches::usage` cannot have a thin wrapper around `App::generate_usage`. - `App::write_*`: getting lazy, didn't seem like high value functions - Any `Settings` (some things need to be figured out here) This is a part of clap-rs#2617

This gives users the basic error template for quick and dirty messages. In addition to the lack of customization, they are not given anything to help them with coloring or for programmayic use (info, source). This is something I've wanted many times for one-off validation that can't be expressed with clap's validation or it just wasn't worth the hoops. The more pressing need is for clap-rs#2255, I need `clap_derive` to be able to create error messages and `Error::with_description` seemed too disjoint from the rest of the clap experience that it seemed like users would immediately create issues about it showing up. With this available, I've gone ahead and deprecated `Error::with_description`, assuming this will be sufficient for users needs (or they can use IO Errors as a back door). I did so according to the pattern in clap-rs#2718 despite us not being fully resolved on that approach yet.

This gives users the basic error template for quick and dirty messages. In addition to the lack of customization, they are not given anything to help them with coloring or for programmayic use (info, source). This is something I've wanted many times for one-off validation that can't be expressed with clap's validation or it just wasn't worth the hoops. The more pressing need is for clap-rs#2255, I need `clap_derive` to be able to create error messages and `Error::with_description` seemed too disjoint from the rest of the clap experience that it seemed like users would immediately create issues about it showing up. With this available, I've gone ahead and deprecated `Error::with_description` (added in 58512f2), assuming this will be sufficient for users needs (or they can use IO Errors as a back door). I did so according to the pattern in clap-rs#2718 despite us not being fully resolved on that approach yet.

epage · 2021-10-16T17:52:06Z

bors r+

This gives users the basic error template for quick and dirty messages. In addition to the lack of customization, they are not given anything to help them with coloring or for programmayic use (info, source). This is something I've wanted many times for one-off validation that can't be expressed with clap's validation or it just wasn't worth the hoops. The more pressing need is for clap-rs#2255, I need `clap_derive` to be able to create error messages and `Error::with_description` seemed too disjoint from the rest of the clap experience that it seemed like users would immediately create issues about it showing up. With this available, I've gone ahead and deprecated `Error::with_description` (added in 58512f2), assuming this will be sufficient for users needs (or they can use IO Errors as a back door). I did so according to the pattern in clap-rs#2718 despite us not being fully resolved on that approach yet.

bors · 2021-10-16T18:58:20Z

Build succeeded:

CI

This gives users the basic error template for quick and dirty messages. In addition to the lack of customization, they are not given anything to help them with coloring or for programmayic use (info, source). This is something I've wanted many times for one-off validation that can't be expressed with clap's validation or it just wasn't worth the hoops. The more pressing need is for clap-rs#2255, I need `clap_derive` to be able to create error messages and `Error::with_description` seemed too disjoint from the rest of the clap experience that it seemed like users would immediately create issues about it showing up. With this available, I've gone ahead and deprecated `Error::with_description` (added in 58512f2), assuming this will be sufficient for users needs (or they can use IO Errors as a back door). I did so according to the pattern in clap-rs#2718 despite us not being fully resolved on that approach yet.

pksunkara · 2021-10-18T09:49:48Z

src/build/arg/mod.rs

+    /// Deprecated, see [`Arg::forbid_empty_values`]
+    #[deprecated(since = "3.0.0", note = "Replaced with `Arg::forbid_empty_values`")]
+    pub fn empty_values(self, empty: bool) -> Self {
+        self.forbid_empty_values(!empty)


This is not correct btw, Check the old code in v2

Please, when you see a problem, say what the problem is, rather than requiring the person to diff to very different code bases in hopes of seeing what the difference was.

From my reading of clap2, I have not seen what is incorrect about it. I'm glad you identified a problem but please help out and say what it is.

I sometimes forget that you don't have the context for what we changed.

Here's the old code which was actually leading to bugs, https://docs.rs/clap/2.33.3/src/clap/args/arg.rs.html#2297-2304.

Let me double check, you are trying to call out that this is missing the "forbidding empty values implies taking a value" part?

If so, I'll get that noted on #2617 to be looked at as part of the wider inference look.

Yes.

I'll get that noted on #2617 to be looked at as part of the wider inference look.

What do you mean by that? In case you mean that we want to add inference back into the settings, then I disagree because that caused a lot of bugs.

epage force-pushed the 2to3 branch 3 times, most recently from 753ee04 to b0ff215 Compare August 18, 2021 18:54

epage force-pushed the 2to3 branch from b0ff215 to 440eccc Compare September 8, 2021 18:29

epage force-pushed the 2to3 branch 4 times, most recently from 533e480 to 3bdee37 Compare October 7, 2021 23:56

epage mentioned this pull request Oct 9, 2021

Color should be exposed as a single enum, not individual Settings #2811

Closed

epage force-pushed the 2to3 branch from 3bdee37 to 7ee80a9 Compare October 12, 2021 12:48

epage added the M-unreviewed Meta: Request for post-merge review. label Oct 12, 2021

bors bot added a commit that referenced this pull request Oct 12, 2021

Merge #2718

71408e9

2718: fix: Ease clap2->clap3 migration with deprecations r=epage a=epage Co-authored-by: Ed Page <eopage@gmail.com>

epage force-pushed the 2to3 branch from 7ee80a9 to a43da7c Compare October 15, 2021 19:19

epage mentioned this pull request Oct 16, 2021

feat: Expose clap-style errors to users #2890

Merged

pksunkara removed the M-unreviewed Meta: Request for post-merge review. label Oct 16, 2021

bors bot merged commit 8647302 into clap-rs:master Oct 16, 2021

epage deleted the 2to3 branch October 16, 2021 20:51

pksunkara reviewed Oct 18, 2021

View reviewed changes

pksunkara mentioned this pull request Oct 18, 2021

Reduce clap2->clap3 porting burden #2617

Closed

2 tasks

epage mentioned this pull request Nov 18, 2021

App::about and Arg::help are inconsistent epage/clapng#16

Open

epage mentioned this pull request Dec 7, 2021

App::about and Arg::help are inconsistent #3075

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: Ease clap2->clap3 migration with deprecations #2718

fix: Ease clap2->clap3 migration with deprecations #2718

epage commented Aug 18, 2021 •

edited

Loading

epage commented Sep 8, 2021

pksunkara commented Oct 1, 2021

epage commented Oct 12, 2021

pksunkara commented Oct 12, 2021

epage commented Oct 12, 2021

bors bot commented Oct 12, 2021

epage commented Oct 12, 2021

epage commented Oct 16, 2021

bors bot commented Oct 16, 2021

pksunkara Oct 18, 2021

epage Oct 18, 2021

pksunkara Oct 18, 2021

epage Oct 19, 2021

pksunkara Oct 19, 2021

fix: Ease clap2->clap3 migration with deprecations #2718

fix: Ease clap2->clap3 migration with deprecations #2718

Conversation

epage commented Aug 18, 2021 • edited Loading

epage commented Sep 8, 2021

pksunkara commented Oct 1, 2021

epage commented Oct 12, 2021

pksunkara commented Oct 12, 2021

epage commented Oct 12, 2021

bors bot commented Oct 12, 2021

epage commented Oct 12, 2021

epage commented Oct 16, 2021

bors bot commented Oct 16, 2021

pksunkara Oct 18, 2021

Choose a reason for hiding this comment

epage Oct 18, 2021

Choose a reason for hiding this comment

pksunkara Oct 18, 2021

Choose a reason for hiding this comment

epage Oct 19, 2021

Choose a reason for hiding this comment

pksunkara Oct 19, 2021

Choose a reason for hiding this comment

epage commented Aug 18, 2021 •

edited

Loading