Deprecate Arg::help in favour of Arg::about [WIP] #1840

creativcoder · 2020-04-19T11:45:21Z

Taking this incrementally as this is a big refactor.
Change: Updated usage of Arg::help with Arg::about, deprecating help

Closes #1823

CreepySkeleton

Looks good so far, keep it up!

src/build/arg/mod.rs

pksunkara

I don't see Arg::about fn being added. Please rename Arg::help fn to it and add a deprecated function:

pub fn help(mut self, h: &'help str) -> Self {
    self.about(h)
}

creativcoder · 2020-04-21T16:08:51Z

Hey @pksunkara I have rebased and added more commits, but seems like this page not reflecting updates from arg_help branch.

Update: Github was down

creativcoder · 2020-04-21T16:46:12Z

So, there's one failing test on the clap_derive crate in doc-comments-help.rs
I have renamed usage of "help" in clap_derive to "about" where I felt it was applicable. Anything else that I missed?

pksunkara

You missed some changes in Arg::from_yaml.

Could you also remove Arg::help and Arg::long_help descriptions?

pksunkara · 2020-04-21T16:52:06Z

clap_derive/src/derives/attrs.rs

-                    || m.name == "long_help"
-                    || m.name == "about"
-                    || m.name == "long_about"
+                m.name == "about" || m.name == "long_about"


You need to also keep the help and long_help here. That should fix the test.

I added help and long_help back, but it still fails. Note that I had updated the test help_is_better_than_comments case to use about attribute. So it shouldn't depend on help; right?

You are right. I misunderstood the test. This code is correct.

But at the same time: top_long_doc_comment_both_help_long_help test passes and it uses about attribute, which is strange

It uses a doc comment. Doc comments fly under help and long_help. Replace this

clap/clap_derive/src/derives/attrs.rs

Line 436 in 3de8af4

res.push_doc_comment(&field.attrs, "help");

With "about" and it should work.

I will take a look at this today and get back to you. Thanks for the patience.

So, I found the issue. Attrs::from_field calls this function. When about is encountered, instead of pushing a method, we are assigning it to Attrs.about here. So, later when we are searching for about methods, we can't see the attribute because it's not a Method in Attrs.methods. You can fix this issue by removing about from Attrs and pushing the about attribute as a method when parsing it.

You would probably have to change the order of these 2 lines afterwards. Similarly, with Attrs::from_field

You can fix this issue by removing about from Attrs and pushing the about attribute as a method when parsing it.

No wait! I was the one who wrote this code and thought I don't remember particulars, I remember it was about something important.

I'll see what is it about tomorrow.

I checked a bit and as far as I can see, it wasn't really needed. @creativcoder should be coming up with any issues if they pop up.

@creativcoder Also, you need to edit the benches folder. They still have uses Arg::help.

I remember now: it was about no_version. We removed this hack in clap_derive so it shuld be fine.

src/build/arg/mod.rs

clap_derive/src/derives/attrs.rs

creativcoder · 2020-04-26T21:16:16Z

9c8c06d has commits only for handling about method resolution. @pksunkara r? Will make changes for remaining comments after this.

pksunkara · 2020-04-26T21:17:39Z

Commit looks good

pksunkara · 2020-04-27T08:36:52Z

Can we finish this up soon? I want to do some changes based on this. Thanks.

creativcoder · 2020-04-27T14:33:32Z

@pksunkara remaining comments have been addressed. r?

Could you also remove Arg::help and Arg::long_help descriptions?

Do you mean removing the docs over them?

pksunkara · 2020-04-27T14:46:28Z

Looks good, just one final thing, you need to change help in https://github.com/clap-rs/clap/blob/master/tests/fixtures/app.yml to about. Similarly, https://github.com/clap-rs/clap/blob/master/tests/fixtures/app_2space.yml

creativcoder · 2020-04-27T14:50:37Z

Done

pksunkara · 2020-04-27T14:58:01Z

bors r+

bors · 2020-04-27T14:58:04Z

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

bors · 2020-04-27T15:08:09Z

👎 Rejected by PR status

pksunkara · 2020-04-27T15:09:42Z

Please format your code and fix clippy warnings.

pksunkara · 2020-04-27T15:23:58Z

bors r+

bors · 2020-04-27T15:24:00Z

🕐 Waiting for PR status (Github check) to be set, probably by CI. Bors will automatically try to run when all required PR statuses are set.

CreepySkeleton · 2020-04-27T16:31:54Z

bors r=pksunkara

bors · 2020-04-27T16:52:36Z

Build succeeded:

This reverts commits 24cb8b1..d0abb37 from clap-rs/clap#1840

This reverts commits 24cb8b1..d0abb37 from clap-rs/clap#1840 This is part of #16. clap-rs/clap#1840 wasn't the right call but we don't have time to make the decision now, so instead of having one option and changing it in 4.0, this reverts back to clap2 behavior.

CreepySkeleton reviewed Apr 19, 2020

View reviewed changes

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

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

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

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

pksunkara requested changes Apr 19, 2020

View reviewed changes

creativcoder force-pushed the arg_help branch from 92e8056 to c79d9a5 Compare April 21, 2020 16:18

pksunkara requested changes Apr 21, 2020

View reviewed changes

creativcoder mentioned this pull request Apr 26, 2020

Update tests #1857

Merged

creativcoder and others added 4 commits April 27, 2020 02:42

Deprecate Arg::help in favour of Arg::about

24cb8b1

Code review p1

75f5a9a

Updated test and usage of older help APIs with about

92d5920

Fix test for about method resolution in clap_derive

9c8c06d

creativcoder force-pushed the arg_help branch from c79d9a5 to 9c8c06d Compare April 26, 2020 21:12

creativcoder added 2 commits April 27, 2020 19:37

Code review p2

266dbbe

Update bench tests with about and add inline to about methods

35fdb45

Fix yaml tests

970c1fa

pksunkara approved these changes Apr 27, 2020

View reviewed changes

cargo fmt

d0abb37

bors bot merged commit 4675070 into clap-rs:master Apr 27, 2020

epage added a commit to epage/clapng that referenced this pull request Nov 18, 2021

Revert "Deprecate Arg::help in favour of Arg::about"

b98d785

This reverts commits 24cb8b1..d0abb37 from clap-rs/clap#1840

epage mentioned this pull request Nov 18, 2021

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

Open

epage mentioned this pull request Nov 18, 2021

Revert "Deprecate Arg::help in favour of Arg::about" epage/clapng#17

Merged

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

Deprecate Arg::help in favour of Arg::about [WIP] #1840

Deprecate Arg::help in favour of Arg::about [WIP] #1840

creativcoder commented Apr 19, 2020 •

edited

CreepySkeleton left a comment

pksunkara left a comment

creativcoder commented Apr 21, 2020 •

edited

creativcoder commented Apr 21, 2020

pksunkara left a comment

pksunkara Apr 21, 2020

creativcoder Apr 22, 2020

pksunkara Apr 22, 2020

creativcoder Apr 22, 2020 •

edited

CreepySkeleton Apr 22, 2020 •

edited

pksunkara Apr 23, 2020

pksunkara Apr 23, 2020

CreepySkeleton Apr 23, 2020

pksunkara Apr 23, 2020

CreepySkeleton Apr 27, 2020

creativcoder commented Apr 26, 2020

pksunkara commented Apr 26, 2020

pksunkara commented Apr 27, 2020

creativcoder commented Apr 27, 2020

pksunkara commented Apr 27, 2020

creativcoder commented Apr 27, 2020

pksunkara commented Apr 27, 2020

bors bot commented Apr 27, 2020

bors bot commented Apr 27, 2020

pksunkara commented Apr 27, 2020

pksunkara commented Apr 27, 2020

bors bot commented Apr 27, 2020

CreepySkeleton commented Apr 27, 2020

bors bot commented Apr 27, 2020

Deprecate Arg::help in favour of Arg::about [WIP] #1840

Deprecate Arg::help in favour of Arg::about [WIP] #1840

Conversation

creativcoder commented Apr 19, 2020 • edited

CreepySkeleton left a comment

Choose a reason for hiding this comment

pksunkara left a comment

Choose a reason for hiding this comment

creativcoder commented Apr 21, 2020 • edited

creativcoder commented Apr 21, 2020

pksunkara left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

creativcoder Apr 22, 2020 • edited

Choose a reason for hiding this comment

CreepySkeleton Apr 22, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

creativcoder commented Apr 26, 2020

pksunkara commented Apr 26, 2020

pksunkara commented Apr 27, 2020

creativcoder commented Apr 27, 2020

pksunkara commented Apr 27, 2020

creativcoder commented Apr 27, 2020

pksunkara commented Apr 27, 2020

bors bot commented Apr 27, 2020

bors bot commented Apr 27, 2020

pksunkara commented Apr 27, 2020

pksunkara commented Apr 27, 2020

bors bot commented Apr 27, 2020

CreepySkeleton commented Apr 27, 2020

bors bot commented Apr 27, 2020

creativcoder commented Apr 19, 2020 •

edited

creativcoder commented Apr 21, 2020 •

edited

creativcoder Apr 22, 2020 •

edited

CreepySkeleton Apr 22, 2020 •

edited