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

fix: set correct field in MietteHandlerOpts::ansi_colors #150

Merged
merged 1 commit into from
Apr 14, 2022

Conversation

sunshowers
Copy link
Contributor

Thanks for miette! We've been having a great time using it in
cargo-nextest to print out nicely-formatted errors.

I noticed a bug while testing out miette -- the ansi_colors method
does not set ANSI colors properly.

By the way, I do not believe miette should use RGB colors at all --
default colors in applications must be restricted to 12 ANSI colors. See
https://rust-cli-recommendations.sunshowers.io/colors.html#color-palettes
for why. I'm happy to disable RGB colors in all the applications I
maintain, but doing so globally is something for upstream to consider.

Thanks for miette! We've been having a great time using it in
cargo-nextest to print out nicely-formatted errors.

I noticed a bug while testing out miette -- the `ansi_colors` method
does not set ANSI colors properly.

By the way, I do not believe miette should use RGB colors at all --
default colors in applications must be restricted to 12 ANSI colors. See
https://rust-cli-recommendations.sunshowers.io/colors.html#color-palettes
for why. I'm happy to disable RGB colors in all the applications I
maintain, but doing so globally is something for upstream to consider.
Copy link
Owner

@zkat zkat left a comment

Choose a reason for hiding this comment

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

wow, copy/paste is incredibly rude. Absolutely terrible.

Thanks for this patch!

@zkat
Copy link
Owner

zkat commented Apr 14, 2022

also re: ansi colors

That's a pity, re: ansi colors! I spent a good amount of time making sure the default colors were both attractive and colorblindness friendly, both of which I think the default ANSI color set kinda... fails to do in most cases.

That said, perhaps you're right and it shouldn't be the default. I'd be happy to take a patch that makes ANSI the default, but the specifics might warrant some discussion. Happy to see a concrete proposal, though!

@zkat zkat merged commit 9719760 into zkat:main Apr 14, 2022
@sunshowers
Copy link
Contributor Author

Thanks for considering the change to ANSI colors!

That's a pity, re: ansi colors! I spent a good amount of time making sure the default colors were both attractive and colorblindness friendly, both of which I think the default ANSI color set kinda... fails to do in most cases.

I think that is definitely true for the default color scheme in most terminals, but the benefit of them is that users can customize them arbitrarily. I would imagine that most colorblind users would have set up an appropriate theme already (though I don't have data to support this!).

I think (as in the color palette link from my recommendations above) that the benefit of being able to customize colors in a global fashion is precisely the benefit that ANSI colors offer and that's harder to access with RGB colors.

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