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.

Sign up for GitHub

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

Jump to bottom

Consider accepting a Cow instead of Into<&'b str> in App::about #2150

Closed
d-e-s-o opened this issue Oct 4, 2020 · 5 comments · Fixed by #4103
Closed

Consider accepting a Cow instead of Into<&'b str> in App::about #2150

d-e-s-o opened this issue Oct 4, 2020 · 5 comments · Fixed by #4103
Labels
A-builder Area: Builder API C-enhancement Category: Raise on the bar on expectations E-medium Call for participation: Experience needed to fix: Medium / intermediate M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Milestone
4.0

Comments

@d-e-s-o
Copy link

d-e-s-o commented Oct 4, 2020

I find the signature of methods like App::about not super useful. It's probably fine when doing everything statically, because you could easily make sure that the string provided outlives the App object (because most of the time you have a 'static string anyway as everything is defined statically in the code). But consider the case where you'd want to add subcommands dynamically at runtime. It gets unnecessarily unwieldy now, in my opinion, because of the lifetime requirement.

Couldn't you instead accept a Cow<'b, str>? This way, users that do something more dynamic would at least have the possibility to fudge that with a dynamic memory allocation at the call site, instead of doing some dance to make the generated string outlive the App object or even boxing the string up just to leak it.
@d-e-s-o d-e-s-o added the C-bug Category: Updating dependencies label Oct 4, 2020
@pksunkara pksunkara added this to the 3.0 milestone Oct 4, 2020
@pksunkara pksunkara added C-enhancement Category: Raise on the bar on expectations and removed C-bug Category: Updating dependencies labels Oct 4, 2020
@ldm0
Copy link
Member

ldm0 commented Oct 6, 2020

Related: #1728

@pksunkara
Copy link
Member

@d-e-s-o Do you think you can take a small attempt at this by only changing the about? Would like to see how it improves the API.

@d-e-s-o d-e-s-o mentioned this issue May 27, 2021
Closed
@d-e-s-o
Copy link
Author

d-e-s-o commented May 27, 2021

@d-e-s-o Do you think you can take a small attempt at this by only changing the about? Would like to see how it improves the API.

Sure, see #2504.

@pksunkara pksunkara removed this from the 3.0 milestone Jun 9, 2021
@1st1 1st1 mentioned this issue Aug 10, 2021
Merged
@epage
Copy link
Member

epage commented Oct 4, 2021

This is related to my proposed fix for #1041

@pksunkara pksunkara added this to the 4.0 milestone Oct 9, 2021
@epage epage mentioned this issue Oct 12, 2021
Closed
@epage epage added the M-breaking-change Meta: Implementing or merging this will introduce a breaking change. label Oct 19, 2021
@epage epage mentioned this issue Dec 6, 2021
Open
@epage epage added A-builder Area: Builder API E-medium Call for participation: Experience needed to fix: Medium / intermediate labels Dec 13, 2021
@epage
Copy link
Member

epage commented Dec 14, 2021

A challenge is help_heading takes Into<Option<&str>>. If we make this Into<Option<Into<CustomStr>>> then None gets an ambiguous type compiler error.

Calling with None might be rare enough that its ok. We'll have to evaluate this.

epage added a commit to epage/clap that referenced this issue Dec 14, 2021
@epage
feat!: Allow resetting abouts/help to default
dd499ab 
I dislike the inconsistency with only a few fields providing this (this
and `help_heading`) but this is to address a specific bug.  We need to
visit this, along with iterators (clap-rs#2870) and string handling (clap-rs#2150).

`Arg` came along for the ride because the derive logic is applied to
both.  `PossibleValue` didn't need it because we filter out `long_help`.

BREAKING CHANGE: We changed the signatures for `App::about`,
`App::long_about`, `Arg::help`, and `Arg::long_help` from accepting anything `Into<&str>` to `&str`.
epage added a commit to epage/clap that referenced this issue Dec 14, 2021
@epage
feat!: Allow resetting abouts/help to default
8ec6f2e 
I dislike the inconsistency with only a few fields providing this (this
and `help_heading`) but this is to address a specific bug.  We need to
visit this, along with iterators (clap-rs#2870) and string handling (clap-rs#2150).

`Arg` came along for the ride because the derive logic is applied to
both.  `PossibleValue` didn't need it because we filter out `long_help`.

BREAKING CHANGE: We changed the signatures for `App::about`,
`App::long_about`, `Arg::help`, and `Arg::long_help` from accepting
anything `Into<&str>` to `&str`.
epage added a commit to epage/clap that referenced this issue Dec 14, 2021
@epage
feat!: Allow resetting abouts/help to default
80f426a 
I dislike the inconsistency with only a few fields providing this (this
and `help_heading`) but this is to address a specific bug.  We need to
visit this, along with iterators (clap-rs#2870) and string handling (clap-rs#2150).

`Arg` came along for the ride because the derive logic is applied to
both.  `PossibleValue` didn't need it because we filter out `long_help`.

BREAKING CHANGE: We changed the signatures for `App::about`,
`App::long_about`, `Arg::help`, and `Arg::long_help` from accepting
anything `Into<&str>` to `&str`.
@epage epage mentioned this issue Dec 14, 2021
Closed
2 tasks
This was referenced May 18, 2022
Closed
Closed
@Jedsek Jedsek mentioned this issue Aug 22, 2022
Merged
epage added a commit to epage/clap that referenced this issue Aug 22, 2022
@epage
fix: Switch to owned types
fe78015 
Impact:
- Binary size: 556.6 KiB to 578.4 KiB
- build time: 6.4950 us (7% slower)
- parse time: 7.7256 us
- parse sc time: 8.1580 us (5% faster)

Fixes clap-rs#1041
Fixes clap-rs#2150
@epage epage mentioned this issue Aug 22, 2022
Merged
epage added a commit to epage/clap that referenced this issue Aug 22, 2022
@epage
fix: Switch to owned types
85f541d 
Impact:
- Binary size: 556.6 KiB to 578.4 KiB
- build time: 6.4950 us (7% slower)
- parse time: 7.7256 us
- parse sc time: 8.1580 us (5% faster)

Fixes clap-rs#1041
Fixes clap-rs#2150
@samkim-crypto samkim-crypto mentioned this issue Mar 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-builder Area: Builder API C-enhancement Category: Raise on the bar on expectations E-medium Call for participation: Experience needed to fix: Medium / intermediate M-breaking-change Meta: Implementing or merging this will introduce a breaking change.
Projects
None yet
Milestone
4.0
Development

Successfully merging a pull request may close this issue.

fix: Switch to owned types epage/clap
4 participants
@epage @pksunkara @d-e-s-o @ldm0