sharkdp / pastel

A command-line tool to generate, analyze, convert and manipulate colors
Apache License 2.0
5.08k stars 102 forks source link

Use PASTEL_COLOR_MODE in ansi::Brush::from_environment #168

Closed Divoolej closed 2 years ago

Divoolej commented 2 years ago

This intends to resolve #121 and continues the work from #140. @sharkdp

Fixes the issue where the pick command only relied on the COLORTERM variable instead of looking for PASTEL_COLOR_MODE first.

Caveats:

sharkdp commented 2 years ago

Thank you very much for your contribution!

A similar block of code exists in cli/main.rs but uses global_matches which are not available for this method, so I'm not sure if avoiding this duplication is possible.

I think ideally, we would add a impl block for ansi::Mode with a from_str method that would return an Option<Self> (with Self = ansi::Mode). We could then use this in main.rs and here, where both parts of the code could still handle the None case separately.

There is no error handling in case of incorrect PASTEL_COLOR_MODE values - I decided to ignore this variable in such cases. In other places it's done by returning PastelError::UnknownColorMode but I couldn't do it here because Brush::from_environment doesn't return Result.

maybe we could briefly look into that and check if it would be a huge effort to let Brush::from_environment return a Result?

Divoolej commented 2 years ago

@sharkdp Thanks for your input! I've pushed some changes that should address your feedback, here are some additional thoughts:

I think ideally, we would add a impl block for ansi::Mode with a from_str method that would return an Option\<Self>

I made this work but had to use Result<Option\<Self>> as the return type, otherwise we'd still need to handle the invalid string case outside the function and it could be messy.

maybe we could briefly look into that and check if it would be a huge effort to let Brush::from_environment return a Result

It was actually pretty straightforward, apart from the function write_stderr, which is already being used inside error handling logic, so throwing a Result there would be cumbersome.

sharkdp commented 2 years ago

I think ideally, we would add a impl block for ansi::Mode with a from_str method that would return an Option

I made this work but had to use Result<Option> as the return type, otherwise we'd still need to handle the invalid string case outside the function and it could be messy.

Could you have a look at the clippy warning? Either rename the method or implement FromStr for Option<ansi::Mode>, if that's possible.

maybe we could briefly look into that and check if it would be a huge effort to let Brush::from_environment return a Result

It was actually pretty straightforward, apart from the function write_stderr, which is already being used inside error handling logic, so throwing a Result there would be cumbersome.

:+1:

Divoolej commented 2 years ago

@sharkdp I decided to rename the method to from_mode_str, because implementing FromStr for Option\<ansi::Mode> would require the newtype pattern here (I think) and I reckoned it's not worth the additional complexity.

sharkdp commented 2 years ago

Thank you!