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

feat(help): Open the door for user styling in the future #4114

Merged
merged 22 commits into from
Aug 25, 2022

Conversation

epage
Copy link
Member

@epage epage commented Aug 25, 2022

This mostly makes it so we can implement the following without breaking changes

In addition, we fixed a bug where the help version of an arg didn't match the usage.

Binary size went from 562.7 KiB to 565.7 KiB. This both added a lot to the size but also reduced it. A large part of the removal came from removing the textwrap dependency, using only what we need which will hopefully help with #2037. We needed to do this anyways because we needed a way to incrementally wrap text so we could wrap a StyledStr.

In general, the code is much simpler, more flexible, and has less duplication that needed to be kept in sync.

epage added 22 commits August 24, 2022 18:17
This added about 10 KiB to the `.text` which I cannot explain why
This added about 4 KiB to `.text` which makes sense since we duplicated
logic.
The immediate benefit is binary size but this also makes us more
flexible on the implementation, like allowing wrapping of `StyledStr`.

This removed 12 KiB from `.text`

This helps towards clap-rs#1365 and probably clap-rs#2037
This dropped us about 1.8 KiB
This adds about 5 KiB to `.text`
While this doesn't reduce size, it cleans things up to make it easier
This ended up favoring the help implementation
@epage epage merged commit f2e5b06 into clap-rs:master Aug 25, 2022
@epage epage deleted the wrap branch August 25, 2022 18:59
@mgeisler
Copy link
Contributor

We needed to do this anyways because we needed a way to incrementally wrap text so we could wrap a StyledStr.

Hi @epage, I would be interested in hearing why the Fragment trait could not be used for this? I'm using it to implement wrapping of variable-width text here: https://mgeisler.github.io/textwrap/. That is, is allows you to wrap arbitrary "fragments" of text, regardless of what they contain.

I've also implemented wrapping of styled Markdown text in the past: mgeisler/textwrap#140 (comment), which might have come in handy for you.

//! Fork of `textwrap` crate
//!
//! Benefits of forking:
//! - Pull in only what we need rather than relying on the compiler to remove what we don't need
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you not already using the optional Cargo features in Textwrap? As per the documentation, every Textwrap dependency is optional.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not referring to textwrap's optional depednencies but the more general behavior which has more code to it.

@epage
Copy link
Member Author

epage commented Sep 14, 2022

Its not clear how Fragment would help with our use case.

Our use case is more like the markdown example. We have spans of formatted text and newlines may appear anywhere within it. The markdown example feels complicated/brittle/non-obvious. I think I'd prefer our incremental splitting option over that.

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.

2 participants