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

Loosen requirement on using &[] to pass into builder functions #2870

Closed
2 tasks done
epage opened this issue Oct 14, 2021 · 0 comments · Fixed by #4081
Closed
2 tasks done

Loosen requirement on using &[] to pass into builder functions #2870

epage opened this issue Oct 14, 2021 · 0 comments · Fixed by #4081
Labels
A-builder Area: Builder API C-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-blocked Status: Blocked on something else such as an RFC or other implementation work.
Milestone
4.0

Comments

@epage
Copy link
Member

epage commented Oct 14, 2021

Please complete the following tasks

  • I have searched the discussions
  • I have searched the existing issues

Rust Version

rustc 1.53.0 (53cb7b09b 2021-06-17)

Clap Version

v3.0.0-beta.4

Minimal reproducible code

See #2857

Steps to reproduce the bug with the above code

See #2857

Actual Behaviour

Builders only accept &[]

Expected Behaviour

Builders accept an IntoIterator

Additional Context

Prior attempts

In #1041 I proposed we use kstring crate for our strings. kstring::KString is like a Cow<'static, str> with small-string optimization. This removes lifetimes from our types but more relevant to here, offers us more Into flexibility that makes it so we can trivial implement the API we want (see #2857 for some of the problems).

Blocked on #1041

Debug Output

No response
@epage epage added C-bug Category: Updating dependencies C: args A-builder Area: Builder API labels Oct 14, 2021
@epage epage added this to the 4.0 milestone Oct 14, 2021
@epage epage added the M-breaking-change Meta: Implementing or merging this will introduce a breaking change. label Oct 19, 2021
@pksunkara pksunkara added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed M: blocked labels Oct 27, 2021
@epage epage mentioned this issue Dec 6, 2021
Open
2 tasks
@epage epage removed the C: args label Dec 8, 2021
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 added a commit to epage/clap that referenced this issue Aug 12, 2022
@epage
fix!: Switch from &[] to IntoIterator
f84e38a 
This is a part of clap-rs#2870 and is prep for clap-rs#1041

Oddly enough, this dropped the binary size by 200 Bytes

Compared to `HEAD~` on `06_rustup`:
- build: 6.21us -> 6.23us
- parse: 7.55us -> 8.17us
- parse_sc: 7.95us -> 7.65us
@epage epage mentioned this issue Aug 12, 2022
Merged
epage added a commit to epage/clap that referenced this issue Aug 15, 2022
@epage
fix!: Prefer IntoIterator over &[]
2de5919 
The main breakinge change cases:
- `&[char]`: now requires removing `&`
- All other non-ID `&[_]`: hopefully clap-rs#1041 will make these non-breaking

Fixes clap-rs#2870
@epage epage mentioned this issue Aug 15, 2022
Merged
@pksunkara pksunkara mentioned this issue Oct 22, 2022
Closed
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-bug Category: Updating dependencies M-breaking-change Meta: Implementing or merging this will introduce a breaking change. S-blocked Status: Blocked on something else such as an RFC or other implementation work.
Projects
None yet
Milestone
4.0
Development

Successfully merging a pull request may close this issue.

fix!: Prefer IntoIterator over &[] epage/clap
2 participants
@epage @pksunkara