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

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

Merged
merged 3 commits into from
Aug 9, 2022

Conversation

Divoolej
Copy link
Contributor

@Divoolej Divoolej commented Jul 3, 2022

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:

  • 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.
  • 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.

@sharkdp
Copy link
Owner

sharkdp commented Jul 17, 2022

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
Copy link
Contributor Author

Divoolej commented Aug 3, 2022

@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
Copy link
Owner

sharkdp commented Aug 4, 2022

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.

👍

@Divoolej
Copy link
Contributor Author

Divoolej commented Aug 9, 2022

@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 sharkdp merged commit a0aa422 into sharkdp:master Aug 9, 2022
@sharkdp
Copy link
Owner

sharkdp commented Aug 9, 2022

Thank you!

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.

pastel pick doesn't display all colors
2 participants