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 color support (+Add Windows Support) #836

Open
kbknapp opened this Issue Jan 30, 2017 · 9 comments

Comments

3 participants
@kbknapp
Member

kbknapp commented Jan 30, 2017

The formatting really needs some love, and adding Windows support would be a dream.

This is the tracking issues for The Great Color Formatting Refactor as it will henceforth be thou known.

I plan to take all cues from BurntSushi/ripgrep's handling of color formatting.


Another thing that'd be great to add, but is not the focus for this issue is the ability to change the colors and formats. Just something to keep in mind while working.

It'd also be nice to get rid of the Format enums and use atty (Done in #862)

@kbknapp

This comment has been minimized.

Show comment
Hide comment
@kbknapp

kbknapp Feb 9, 2017

Member

I've began work on this issue - I'm hoping to have something mergable within the week.

Member

kbknapp commented Feb 9, 2017

I've began work on this issue - I'm hoping to have something mergable within the week.

@kbknapp

This comment has been minimized.

Show comment
Hide comment
@kbknapp

kbknapp Feb 9, 2017

Member

@pkgw I don't have a public branch yet. I've just been mocking it out, I literally just started messing with it today ;)

Moving the conversation here so as not to clog up ripgrep traffic.

Member

kbknapp commented Feb 9, 2017

@pkgw I don't have a public branch yet. I've just been mocking it out, I literally just started messing with it today ;)

Moving the conversation here so as not to clog up ripgrep traffic.

@kbknapp kbknapp added this to the 2.21.0 milestone Feb 12, 2017

@kbknapp kbknapp moved this from Triaged to Up Next in Status Feb 12, 2017

@pkgw

This comment has been minimized.

Show comment
Hide comment
@pkgw

pkgw Feb 19, 2017

Contributor

I started taking a look at this. I've seen at least a few places where String types would need to be replaced with termcolor::Buffer types to get proper colorized output. This raises a question: would it be OK to have clap always depend on the termcolor crate, even when the color feature is turned off? If yes, life will be a bit easier for implementing the refactored color support. If it's important for that dependency to be optional, I think the cleanest approach to the no-color case would be to create some dummy types that emulate those of termcolor.

Contributor

pkgw commented Feb 19, 2017

I started taking a look at this. I've seen at least a few places where String types would need to be replaced with termcolor::Buffer types to get proper colorized output. This raises a question: would it be OK to have clap always depend on the termcolor crate, even when the color feature is turned off? If yes, life will be a bit easier for implementing the refactored color support. If it's important for that dependency to be optional, I think the cleanest approach to the no-color case would be to create some dummy types that emulate those of termcolor.

pkgw added a commit to pkgw/clap-rs that referenced this issue Feb 19, 2017

refactor: use the atty crate for isatty() detection
Not only does this remove some unsafe code from clap itself, `atty` does the
right thing on Windows too. This isn't relevant now since we don't currently
support colorized output on Windows, but will come in handy if/when we
implement that feature (clap-rs#836).
@BurntSushi

This comment has been minimized.

Show comment
Hide comment
@BurntSushi

BurntSushi Feb 19, 2017

Contributor

@pkgw Out of curiosity, why do you need the Buffer type? At least as I initially envisioned it, that particular type is most useful in multithreaded contexts.

Contributor

BurntSushi commented Feb 19, 2017

@pkgw Out of curiosity, why do you need the Buffer type? At least as I initially envisioned it, that particular type is most useful in multithreaded contexts.

@pkgw

This comment has been minimized.

Show comment
Hide comment
@pkgw

pkgw Feb 19, 2017

Contributor

@BurntSushi There are functions in clap that return Strings containing buffered, colorized output ... because they bake in the assumption that you can implement colors with ANSI escape sequences. If I'm understanding the termcolor architecture right, Buffer helps in exactly this sort of situation even if you're not threading. But I don't know the design of clap very well, so perhaps it wouldn't be hard to change these functions to print their output directly, rather than buffering things.

Contributor

pkgw commented Feb 19, 2017

@BurntSushi There are functions in clap that return Strings containing buffered, colorized output ... because they bake in the assumption that you can implement colors with ANSI escape sequences. If I'm understanding the termcolor architecture right, Buffer helps in exactly this sort of situation even if you're not threading. But I don't know the design of clap very well, so perhaps it wouldn't be hard to change these functions to print their output directly, rather than buffering things.

@BurntSushi

This comment has been minimized.

Show comment
Hide comment
@BurntSushi

BurntSushi Feb 19, 2017

Contributor

because they bake in the assumption that you can implement colors with ANSI escape sequences

Ah, yup, that's the key I was missing. If that's the case, then Buffer will, incidentally, help. Neat. (Short of restructuring how clap handles colors, as you say.)

Contributor

BurntSushi commented Feb 19, 2017

because they bake in the assumption that you can implement colors with ANSI escape sequences

Ah, yup, that's the key I was missing. If that's the case, then Buffer will, incidentally, help. Neat. (Short of restructuring how clap handles colors, as you say.)

homu added a commit that referenced this issue Feb 19, 2017

Auto merge of #862 - pkgw:pr-atty, r=kbknapp
refactor: use the atty crate for isatty() detection

Not only does this remove some unsafe code from clap itself, `atty` does the right thing on Windows too. This isn't relevant now since we don't currently support colorized output on Windows, but will come in handy if/when we implement that feature (#836).
@kbknapp

This comment has been minimized.

Show comment
Hide comment
@kbknapp

kbknapp Feb 19, 2017

Member

would it be OK to have clap always depend on the termcolor crate, even when the color feature is turned off?

Of course I'd prefer to use dummy types or work around pulling in unneeded deps in that case, but if the dep is small and doesn't bloat a resulting binary excessively I would be willing to consider pulling it in unconditionally.

I guess I'd need a concrete example to really see trade-off between the pain of using dummy types and resulting binary size.

But I don't know the design of clap very well, so perhaps it wouldn't be hard to change these functions to print their output directly, rather than buffering things.

The current coloring support was built around using ansi_term, so if that architecture doesn't fit anymore I'm fine with rebuilding something that fits better so long as it can be done in a non-breaking, net neutral effect way.

Member

kbknapp commented Feb 19, 2017

would it be OK to have clap always depend on the termcolor crate, even when the color feature is turned off?

Of course I'd prefer to use dummy types or work around pulling in unneeded deps in that case, but if the dep is small and doesn't bloat a resulting binary excessively I would be willing to consider pulling it in unconditionally.

I guess I'd need a concrete example to really see trade-off between the pain of using dummy types and resulting binary size.

But I don't know the design of clap very well, so perhaps it wouldn't be hard to change these functions to print their output directly, rather than buffering things.

The current coloring support was built around using ansi_term, so if that architecture doesn't fit anymore I'm fine with rebuilding something that fits better so long as it can be done in a non-breaking, net neutral effect way.

@pkgw

This comment has been minimized.

Show comment
Hide comment
@pkgw

pkgw Feb 19, 2017

Contributor

@kbknapp OK, well, dummy types certainly won't be that painful to implement, and they're unconditionally better once implemented, so it sounds like that's the way to go.

Contributor

pkgw commented Feb 19, 2017

@kbknapp OK, well, dummy types certainly won't be that painful to implement, and they're unconditionally better once implemented, so it sounds like that's the way to go.

@pkgw

This comment has been minimized.

Show comment
Hide comment
@pkgw

pkgw Feb 21, 2017

Contributor

Hmmm, I took more of a look at this and I think that making the change from ansi_term to termcolor would require a lot more knowledge of clap's internals than I have. @kbknapp, this issue is all yours 😄

Contributor

pkgw commented Feb 21, 2017

Hmmm, I took more of a look at this and I think that making the change from ansi_term to termcolor would require a lot more knowledge of clap's internals than I have. @kbknapp, this issue is all yours 😄

@kbknapp kbknapp moved this from Up Next to Triaged in Status Mar 27, 2017

@kbknapp kbknapp referenced this issue Aug 21, 2017

Open

Tracking Issue for 3.x #1037

51 of 90 tasks complete

@kbknapp kbknapp modified the milestones: 2.21.0, v3-alpha1 Feb 2, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment