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

Refactor: Builder pattern methods receiving str array parameters to instead take ref iterators #2414

Conversation

MS-Painter
Copy link

@MS-Painter MS-Painter commented Mar 13, 2021

Currently, builder pattern Arg methods which allow inserting multiple values,
receive parameters of type &[&'... str] . This can be an inconvenience to work with, for example -
When attempting to pass to those methods data that isn't manually hard-coded.

This PR allows utilizing these methods as usual but also allows more freedom to receive other types.

TODO

  • Refactor all relevant methods.
  • Fix issue between refactor and derive macro pattern apps (Issues in the tests utilizing said pattern (Cause - ArgEnum procedural macro).

src/build/arg/mod.rs Outdated Show resolved Hide resolved
@pksunkara pksunkara added this to the 3.0 milestone Aug 13, 2021
@kbknapp
Copy link
Member

kbknapp commented Oct 5, 2021

In general I get a little nervous when adding generic arguments being that the number one and two complaints about clap are it's binary size and compile times...both of which are increased by generic arguments. However in a case like this, the methods are small enough that the addition should be negligible for a pretty clear ergonomic win.

epage added a commit to epage/clap that referenced this pull request Oct 12, 2021
This replaces the most common uses of `&[]` with `IntoIterator`.

This is based on clap-rs#2414.  Unlike that, this stays consistent with using
`Into<&'help str>`.

There are some remaining functions not converted over, like defaults and
requires.

BREAKING CHANGE: Instances like `aliases(&[])` need to switch to
`aliases([])`.
@epage
Copy link
Member

epage commented Oct 12, 2021

Recreated this in #2857

@epage epage closed this Oct 12, 2021
@pksunkara pksunkara removed this from the 3.0 milestone Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants