Refactor #1976

CreepySkeleton · 2020-06-18T15:31:57Z

Refactor coloring/help/errors facility.

Improvements

Creation of of clap::Error is now completely separate from actual emitting. It cannot fail anymore.

Significant code bloat decrease:

Before

 File  .text     Size Crate
 9.4%  56.8% 289.5KiB clap
 4.8%  28.7% 146.2KiB std
 1.9%  11.6%  59.1KiB [Unknown]
 0.2%   0.9%   4.8KiB indexmap
 0.1%   0.3%   1.6KiB textwrap
 0.0%   0.1%     307B probe
 0.0%   0.0%      41B os_str_bytes
16.6% 100.0% 509.8KiB .text section size, the file size is 3.0MiB

After

 File  .text     Size Crate
 8.6%  53.7% 262.5KiB clap
 5.0%  31.2% 152.5KiB std
 1.9%  12.1%  59.1KiB [Unknown]
 0.2%   1.0%   4.8KiB indexmap
 0.1%   0.3%   1.6KiB textwrap
 0.0%   0.1%     307B probe
 0.0%   0.0%      41B os_str_bytes
16.1% 100.0% 489.1KiB .text section size, the file size is 3.0MiB

No more our own unicorn reimplementations of str::split. I consider this a huge improvement.

Changes to public API

App::write_[long_]version is replaced with render version. It was necessary in order to simplify some code. I found that these methods are very rarely used, and what those people really needed was String.

Actually, in light of App::get[_long]_version, I think we could just remove it. @pksunkara Any objections?
Error::cause and Error::info fields are removed. I've never seen them used in practice, and they are responsible for about 10 KiB of code bloat.
print_help* and write_help* now return io::Result instead of clap::Result. There's no point to convert them to clap errors.

Performance regression

I haven't done proper benchmarking yet, but I expect certain regression in error/help rendering. I suppose the regression is negligible because printing is slow enough on it's own, but I'm open to continue working on improving it.

pksunkara

Only early review. I will finish the full review once my 2 comments are addressed.

src/parse/parser.rs

src/parse/errors.rs

pksunkara · 2020-07-12T22:52:31Z

Something is bugging me about this PR and I can't exactly pinpoint what it is. Which is leading me to procrastinate on reviewing this. 😞

CreepySkeleton · 2020-07-18T16:04:44Z

ping @pksunkara

This PR is essentially a blocker for any other change to the error facility because it spans over almost thousand of lines of code over there.

pksunkara

Error::cause and Error::info should not be removed. People should have the option to use clap for other kind of stuff too. cause should have plain error message and info should have programmable error information.

I understand the concern about bloat, but we can always hide these if terminal-only (or other appropriately named) feature flag is enabled. But let's decide on that later though separate from this PR.

CreepySkeleton · 2020-07-19T15:18:53Z

It's not just about bloat, it's about feature creep. This is a golden opportunity to prune unused and redundant stuff, would be a real shame to miss it.

Actually, this talk was the reason I removed the fields in the first place - I saw some useless code and wanted to trigger the discussion around it in hope somebody would come with a compelling reason not to remove it. Your argument is basically "let's leave it just in case". Did you ever use them? Can you give an example (some code from a real project, not a hello-world)?

My arguments:

cause is useless because it's err.to_string().split("\n\n").next().unwrap(). Literally. I'm not opposed to having a method encapsulating that - which will be removed by LTO if not used by the way, but I don't see any reason in dedicating a separate field for that and even making use of std::format! which isn't exactly lightweight.

I also don't have a tiniest idea of what it can be used for.
info is useless because I fail to see how it can possibly be used. I have talked to a few people and none of them used these fields and none of them could come up with an idea how-to. While there's obviously sampling bias at play, I think "nobody needs it" is good null hypothesis here.

I'm not so sure on this one in light of imp(errors): Provide the missing required arguments as info #2016 , but that person didn't supply any particular examples either. I believe this is just a miscommunication of sorts and what they want is actually .to_string(). Let's continue the discussion there.

pksunkara · 2020-07-20T10:14:21Z

cause is not useless. If I want to use my CLI in 2 different places (one terminal with colors and one where I don't want the colors (ex: chatroom like described in #2016)), I don't want to run the CLI twice. I would prefer both the error messages to exist in just one run.

CreepySkeleton · 2020-07-26T08:05:56Z

If I want to use my CLI in 2 different places (one terminal with colors and one where I don't want the colors

That piece gives me strong impression that you're mistaking cause for to_string :) How it actually works:

The Display impl doesn't do coloring. Never. This is exactly what you're supposed to use when you want no colors. Colorizer::print is what you use if you want colors (Error::exit uses just that). We'll need to introduce pub fn Error::print(stderr: bool), but I'd like to postpone it for another PR, this one is already hell of a change.

As I noticed earlier, cause is just the first sentence from the rendered error (i.e the message without "did you mean" and "hint" stuff). For what do you think you can use this? And as noted, it can be easily emulated via err.to_string().split("\n\n").next().unwrap().

I accept your reasoning for info, I'll put it back.

src/build/app/mod.rs

pickfire · 2020-08-09T05:26:25Z

@CreepySkeleton Since we have the luxury to change stuff. Can we enable colored help by default? It stays hidden for quite a few crates that depends on clap, I have sent pull requests to some of them and they were surprised by this feature.

CreepySkeleton · 2020-08-09T13:25:15Z

Colored help feature = "color" will be on by default in 3.0.

pickfire · 2020-08-09T14:25:51Z

Colored help feature = "color" will be on by default in 3.0.

Yes, the feature is on by default. IIRC even in 2.0 but I mean AppSettings::ColoredHelp, not the cargo feature but the clap setting to show colored help by default. Looking at clap_derive basic example, cargo run --example basic -- -h does not show any color.

By the way, I have a comment above that you probably missed.

CreepySkeleton · 2020-08-09T14:59:59Z

@pickfire Could you please create a separate issue about coloring? For the record, I agree. The Auto, Never, Always settings set is enough.

src/build/app/mod.rs

src/output/fmt.rs

src/parse/errors.rs

src/output/help.rs

pksunkara · 2020-08-27T08:18:42Z

src/parse/errors.rs

    }

    pub(crate) fn value_validation(
-        arg: Option<&Arg>,
-        err: &str,
+        arg: String,


Why is this not &Arg? Why is val and err not &str?

Because we don't always have the Arg, like in value_of_t.

pksunkara · 2020-08-27T08:19:08Z

src/parse/errors.rs

-        other: Option<O>,
-        usage: U,
+        other: Option<String>,
+        usage: String,


Why is this not &str?

Because what we need here is String, not &str. This way the caller can decide how to form the string and reuse the already formed String without reallocation on our part. In fact, the caller has String most of the time.

I was asking because the usage parameter in all the other errors below has &str but this one was different. I just wanted to confirm that it was intentional since it's not consistent.

Yes, I was going to make the other methods to use String too (I have some ongoing work done locally), but decided to postpone it as quite irrelevant (PR is already larger than necessary). Do you want it in this PR or as a followup?

If it's only &str to String and such minor stuff, you can add a new commit to this PR. We will squash after I review and approve the PR.

pksunkara

Also, can you please squash the commits once the PR is ready. I don't mind if it's just one large commit.

src/output/fmt.rs

pksunkara

You misrebased the tests/help.rs file.

pksunkara · 2020-08-28T05:20:29Z

Your tests are failing because new stable is released. I fixed the UI tests in #2114

pksunkara

Can you please squash now?

pksunkara · 2020-08-28T15:26:46Z

Once squashed, you can do bors r=pksunkara to merge. 👍

CreepySkeleton · 2020-08-28T16:46:52Z

bors r=pksunkara

bors · 2020-08-28T17:39:34Z

Build succeeded:

pksunkara self-requested a review June 18, 2020 18:20

pksunkara requested changes Jun 18, 2020

View reviewed changes

src/parse/parser.rs Outdated Show resolved Hide resolved

src/parse/errors.rs Show resolved Hide resolved

CreepySkeleton force-pushed the refactor branch 2 times, most recently from e7125a4 to 49b5cff Compare July 6, 2020 19:48

CreepySkeleton marked this pull request as ready for review July 6, 2020 19:49

CreepySkeleton requested a review from pksunkara July 6, 2020 19:49

pksunkara requested changes Jul 19, 2020

View reviewed changes

pksunkara mentioned this pull request Jul 19, 2020

imp(errors): Provide the missing required arguments as info #2016

Merged

CreepySkeleton force-pushed the refactor branch from f2eef00 to b2cac73 Compare July 28, 2020 08:25

pickfire reviewed Aug 9, 2020

View reviewed changes

src/build/app/mod.rs Outdated Show resolved Hide resolved

pickfire mentioned this pull request Aug 10, 2020

Clap v3 ColoredHelp by default #2055

Closed

2 tasks

pksunkara reviewed Aug 11, 2020

View reviewed changes

src/build/app/mod.rs Show resolved Hide resolved

CreepySkeleton force-pushed the refactor branch 3 times, most recently from a9f6bde to ca9f300 Compare August 22, 2020 10:26

pksunkara requested changes Aug 27, 2020

View reviewed changes

src/output/fmt.rs Outdated Show resolved Hide resolved

CreepySkeleton force-pushed the refactor branch from ca9f300 to 0b213d8 Compare August 27, 2020 20:23

pksunkara requested changes Aug 27, 2020

View reviewed changes

CreepySkeleton force-pushed the refactor branch from 0b213d8 to 510708f Compare August 28, 2020 15:07

pksunkara approved these changes Aug 28, 2020

View reviewed changes

Refactor coloring

5020333

CreepySkeleton force-pushed the refactor branch from 510708f to 5020333 Compare August 28, 2020 16:22

bors bot merged commit e4b5407 into master Aug 28, 2020

bors bot deleted the refactor branch August 28, 2020 17:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor #1976

Refactor #1976

CreepySkeleton commented Jun 18, 2020 •

edited

Loading

pksunkara left a comment

pksunkara commented Jul 12, 2020

CreepySkeleton commented Jul 18, 2020

pksunkara left a comment •

edited

Loading

CreepySkeleton commented Jul 19, 2020

pksunkara commented Jul 20, 2020 •

edited

Loading

CreepySkeleton commented Jul 26, 2020

pickfire commented Aug 9, 2020 •

edited

Loading

CreepySkeleton commented Aug 9, 2020

pickfire commented Aug 9, 2020 •

edited

Loading

CreepySkeleton commented Aug 9, 2020

pksunkara Aug 27, 2020

CreepySkeleton Aug 27, 2020

pksunkara Aug 27, 2020

CreepySkeleton Aug 27, 2020

pksunkara Aug 27, 2020

CreepySkeleton Aug 27, 2020

pksunkara Aug 27, 2020

pksunkara left a comment

pksunkara left a comment

pksunkara commented Aug 28, 2020

pksunkara left a comment

pksunkara commented Aug 28, 2020

CreepySkeleton commented Aug 28, 2020

bors bot commented Aug 28, 2020

Refactor #1976

Refactor #1976

Conversation

CreepySkeleton commented Jun 18, 2020 • edited Loading

Improvements

Changes to public API

Performance regression

pksunkara left a comment

Choose a reason for hiding this comment

pksunkara commented Jul 12, 2020

CreepySkeleton commented Jul 18, 2020

pksunkara left a comment • edited Loading

Choose a reason for hiding this comment

CreepySkeleton commented Jul 19, 2020

pksunkara commented Jul 20, 2020 • edited Loading

CreepySkeleton commented Jul 26, 2020

pickfire commented Aug 9, 2020 • edited Loading

CreepySkeleton commented Aug 9, 2020

pickfire commented Aug 9, 2020 • edited Loading

CreepySkeleton commented Aug 9, 2020

pksunkara Aug 27, 2020

Choose a reason for hiding this comment

CreepySkeleton Aug 27, 2020

Choose a reason for hiding this comment

pksunkara Aug 27, 2020

Choose a reason for hiding this comment

CreepySkeleton Aug 27, 2020

Choose a reason for hiding this comment

pksunkara Aug 27, 2020

Choose a reason for hiding this comment

CreepySkeleton Aug 27, 2020

Choose a reason for hiding this comment

pksunkara Aug 27, 2020

Choose a reason for hiding this comment

pksunkara left a comment

Choose a reason for hiding this comment

pksunkara left a comment

Choose a reason for hiding this comment

pksunkara commented Aug 28, 2020

pksunkara left a comment

Choose a reason for hiding this comment

pksunkara commented Aug 28, 2020

CreepySkeleton commented Aug 28, 2020

bors bot commented Aug 28, 2020

CreepySkeleton commented Jun 18, 2020 •

edited

Loading

pksunkara left a comment •

edited

Loading

pksunkara commented Jul 20, 2020 •

edited

Loading

pickfire commented Aug 9, 2020 •

edited

Loading

pickfire commented Aug 9, 2020 •

edited

Loading