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

Rename Arg::help to Arg::about #1823

Closed
pksunkara opened this issue Apr 14, 2020 · 10 comments · Fixed by #1840
Closed

Rename Arg::help to Arg::about #1823

pksunkara opened this issue Apr 14, 2020 · 10 comments · Fixed by #1840
Labels
C-enhancement Category: Raise on the bar on expectations E-easy Call for participation: Experience needed to fix: Easy / not much M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Milestone

Comments

@pksunkara
Copy link
Member

Rename Arg::help to Arg::about to make it consistent with App. Similarly Arg::long_help. Also rename the respective field names.

@pksunkara pksunkara added C-enhancement Category: Raise on the bar on expectations M-breaking-change Meta: Implementing or merging this will introduce a breaking change. P2: need to have E-easy Call for participation: Experience needed to fix: Easy / not much labels Apr 14, 2020
@pksunkara pksunkara added this to the 3.0 milestone Apr 14, 2020
@creativcoder
Copy link
Contributor

Hey @pksunkara, Can I take this up?

@CreepySkeleton
Copy link
Contributor

@creativcoder Sure, go ahead!

@Dylan-DPC-zz
Copy link

I don't see the point of renaming this, it will be another change for a user upgrading from 2.x land to 3.x without much reason.

@pksunkara
Copy link
Member Author

There is much reason because help in App is different semantically than help in Arg which would end up confusing the users. This should fix it and make it consistent.

@CreepySkeleton
Copy link
Contributor

As a compromise, we can rename it but leave the old help, deprecating it.

@Dylan-DPC-zz
Copy link

Help is more synonymous with CLIs and the usage is understood. I don't see the value of this tbh

@pksunkara
Copy link
Member Author

pksunkara commented Apr 19, 2020

Help is about the full help message. About concerns only the description.

I don't want to keep Arg::help behaviour the same and keep it deprecated. I want to make it similar to how App::help behaves, by making it replace the full help text of an argument rather than the generated one.

The generated help contains other things like possible values, default values etc.. which should completely be replaced if help was specified.

@CreepySkeleton
Copy link
Contributor

I want to make it similar to how App::help behaves, by making it replace the full help text of an argument rather than the generated one.

So, you are going to change the behavior of a very frequently used function without changing its signature. If you ask me - or don't, but I'll tell you anyway - this is spectacularly bad idea. Just because old habits die hard. This will spread a lot of confusion among users, a lot more than the benefit from having the API unified will ever be. If you want to pull such a drastic change - rename the method.

For the record - I kind of see Dylan's point. People are already got used to about/help difference. But I also see why we may want to make the API consistent.

My call - let's ask for forgiveness, not for permission. We do this, we push the beta outdoors, receive feedback, revert if the feedback turns out to be largely negative.

@pksunkara
Copy link
Member Author

Just because old habits die hard. This will spread a lot of confusion among users

I agree which is why it took me such a long time to create the issue. I had been thinking about this since I started working on clap. It kept bothering me that the help related stuff is very inconsistent between App and Arg. I want to do a HelpBase which extracts the common stuff and make it customisable.

@bors bors bot closed this as completed in #1840 Apr 27, 2020
@epage
Copy link
Member

epage commented Oct 23, 2021

I've had some first-hand confusion over this change and have started #2937 about the specific problem I had, while trying to acknowledge all of the other needs. Hopefully we can find a good solution.

dfinity-bot pushed a commit to dfinity/ic that referenced this issue Mar 29, 2022
bump clap version

Clap moved to stable and introduces some changes.

- `about` now named `help`. [Ref](clap-rs/clap#1823)
- `setting = AppSettings::ColoredHelp` now default
- rename from `Clap` -> `Parser` 

See merge request dfinity-lab/public/ic!4020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: Raise on the bar on expectations E-easy Call for participation: Experience needed to fix: Easy / not much M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants