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

Stablize App::replace Tracking Issue #2836

Closed
1 of 2 tasks
epage opened this issue Oct 9, 2021 · 8 comments · Fixed by #4805
Closed
1 of 2 tasks

Stablize App::replace Tracking Issue #2836

epage opened this issue Oct 9, 2021 · 8 comments · Fixed by #4805
Labels
A-builder Area: Builder API C-tracking-issue Category: A tracking issue for an unstable feature S-blocked Status: Blocked on something else such as an RFC or other implementation work.

Comments

@epage
Copy link
Member

epage commented Oct 9, 2021

Original request: #1603

Original PR: #1697

Feature flag: unstable-replace

Known issues:

@epage epage added C-enhancement Category: Raise on the bar on expectations A-builder Area: Builder API labels Oct 9, 2021
@pksunkara
Copy link
Member

We should link implementation also, right?

@epage
Copy link
Member Author

epage commented Oct 9, 2021

Works for me.

@pksunkara pksunkara added C-tracking-issue Category: A tracking issue for an unstable feature and removed C-enhancement Category: Raise on the bar on expectations labels Oct 12, 2021
@pksunkara
Copy link
Member

So -l expands to --source list and -r expands to --target residues?

The unstable replace feature gets you part way. It would allow a -l to expand out but it wouldn't allow -lr.

Another issue with this feature as the use case is documented here

@epage
Copy link
Member Author

epage commented Mar 23, 2023

Due to the lack of interest on this tracking issue, I'm considering removing this unstable feature.

Any concerns people want to raise?

@pksunkara
Copy link
Member

Would you happen to have ideas on how to solve the mentioned issues? Maybe by restricting replace behavior to only work on subcommands? There aren't any other new issues.

@epage
Copy link
Member Author

epage commented Mar 24, 2023

But the problem in #2011 is with subcommands and I thought they were the primary motivation for this.

There is also the item brought up about being invisible in --help.

I also feel like there has been little interest expressed in this that it makes me wonder if the feature is worth it in the first place.

@pksunkara
Copy link
Member

Yeah, that's what I meant. The current API replaces any and all input. Maybe we can restrict it to only replace subcommands by doing asserts. That, way we can use the replace config to solve the above-mentioned issues.

I also feel like there has been little interest expressed in this that it makes me wonder if the feature is worth it in the first place.

People might not be talking about it because they don't have anything to complain.

That's something we might have different opinions about. I would recommend keeping it.

@epage
Copy link
Member Author

epage commented Mar 24, 2023

Except for the fact that its unstable which carries its own level of risk, including it being removed.

Features carry a cost and we need to balance that cost against the impact they have on end-users. Right now, command aliases can be emulated by duplicating the commands. This is also done in our "default subcommand" example (see the git examples). People requested that workflow to be streamlined but with us having a workaround, the cost seemed hardly justified. I feel like this might be another one of those scenarios.

epage added a commit to epage/clap that referenced this issue Mar 28, 2023
This has been implemented for 3 years without much traction for
finishing it up.

The subcommand use case can be worked around by creating `Command`s that
just include the relevant logic, very similar to the default subcommand
examples in `git` / `git-derive`.

Using this for flags is covered by clap-rs#4793.

Without `unstable-replace` being enabled, this still cut 5 KiB from
`cargo bloat --release --example git`.

Closes clap-rs#2836
Closes clap-rs#2011
@epage epage closed this as not planned Won't fix, can't repro, duplicate, stale Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-builder Area: Builder API C-tracking-issue Category: A tracking issue for an unstable feature S-blocked Status: Blocked on something else such as an RFC or other implementation work.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants