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

error-stack: introduction of color modes #1800

Merged
merged 62 commits into from
Jan 30, 2023
Merged

error-stack: introduction of color modes #1800

merged 62 commits into from
Jan 30, 2023

Conversation

indietyp
Copy link
Member

@indietyp indietyp commented Jan 11, 2023

🌟 What is the purpose of this PR?

This implements the notion of color modes, determining if colors and which type of coloring/styling are requested.

This enables users to override the preferred mode while preserving the convenience of environment variables like NO_COLOR and FORCE_COLOR. This has been moved into an example.

The available color modes are Color, None, and Emphasis. This mode is available for hooks so that they can easily adapt. A perfect example of this is the location hook, which now will use the mode to either print in bright black or italic.

If no color mode is set, the mode is automatically determined using owo-colors using supports-colors and the specified override. If no color mode is set, the mode is set to Color::Emphasis if the color feature (opt-in) is enabled.

Note: as a drive-by, supports-colors is now only enabled on std, supports-color implies std (not as a feature, found this by accident), which meant that no-std colors would fail, even tho they shouldn't.

🔗 Related links

🔍 What does this change?

  • Report::format_color_mode_preference
  • HookContext::mode() is now available

📜 Does this require a change to the docs?

Yes, the new ColorMode needs to be explained. This is already done.

We might want to add additional explanation/info in lib.rs

🏃 Next Steps

  1. Ideally we'd temporarily set the mode from owo-colors, if possible, this needs overwrite color support temporarily during function execution jam1garner/owo-colors#84

⚠️ Known issues

🛡 What tests cover this?

  • Existing tests
  • New snapshot tests

📹 Demo

image

image

@github-actions github-actions bot added the area/libs > error-stack Affects the `error-stack` crate (library) label Jan 11, 2023
@codecov
Copy link

codecov bot commented Jan 11, 2023

Codecov Report

Merging #1800 (8983737) into main (1394be1) will increase coverage by 0.22%.
The diff coverage is 93.40%.

@@            Coverage Diff             @@
##             main    #1800      +/-   ##
==========================================
+ Coverage   57.05%   57.27%   +0.22%     
==========================================
  Files         261      265       +4     
  Lines       18700    18802     +102     
  Branches      421      421              
==========================================
+ Hits        10669    10769     +100     
- Misses       8026     8028       +2     
  Partials        5        5              
Flag Coverage Δ
backend-integration-tests 3.66% <ø> (ø)
error-stack 90.73% <93.40%> (+0.48%) ⬆️
unit-tests 1.49% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
packages/libs/error-stack/src/hook/context.rs 78.89% <ø> (ø)
packages/libs/error-stack/src/hook/mod.rs 100.00% <ø> (ø)
packages/libs/error-stack/src/lib.rs 38.46% <ø> (ø)
packages/libs/error-stack/src/report.rs 85.22% <ø> (ø)
packages/libs/error-stack/src/fmt/override.rs 65.00% <65.00%> (ø)
packages/libs/error-stack/src/fmt/color.rs 90.00% <90.00%> (ø)
packages/libs/error-stack/src/fmt/location.rs 94.44% <90.90%> (+8.73%) ⬆️
packages/libs/error-stack/src/fmt/charset.rs 94.44% <94.44%> (ø)
packages/libs/error-stack/src/fmt/mod.rs 96.49% <98.76%> (+1.36%) ⬆️
packages/libs/error-stack/src/fmt/config.rs 100.00% <100.00%> (ø)
... and 2 more

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

TimDiekmann
TimDiekmann previously approved these changes Jan 30, 2023
@trunk-io
Copy link

trunk-io bot commented Jan 30, 2023

❌ This pull request failed tests. It has been removed from queue. (details)

@indietyp
Copy link
Member Author

/trunk merge

trunk-io bot pushed a commit that referenced this pull request Jan 30, 2023
## 🌟 What is the purpose of this PR?

In various PR the Rust CI linting job is failing as `cargo make` is supposed to be installed, but it's not. This PR tries to solve this by not caching the tools (which is probably a good idea anyway). If this PR is able to be merged, this issue should be solved, otherwise, it's very likely hitting the same bug.

## 🔗  Related links

This error happened in
- #1800
- #1973 
- #1976
trunk-io bot pushed a commit that referenced this pull request Jan 30, 2023
## 🌟 What is the purpose of this PR?

We have a logic in place to only run a subset in PRs and only run the full set of tests before merge. However, in order to test locally, this logic should be reversed and instead of testing for a `push` event, this should test for `pull_request` events.

## 🔗 Related links

- #1800
@trunk-io trunk-io bot merged commit 07a52f3 into main Jan 30, 2023
@trunk-io trunk-io bot deleted the bm/es/color-mode branch January 30, 2023 17:18
@trunk-io
Copy link

trunk-io bot commented Jan 30, 2023

😎 Merged successfully (details).

trunk-io bot pushed a commit that referenced this pull request Jan 31, 2023
## 🌟 What is the purpose of this PR?

Remove `owo-colors` as a dependency. This means we can push the enable `colors` support without an extra feature! `owo-colors` is being replaced with a vendored solution that does the same thing, only more correct.

This also implements the suggestion from #1800 (comment)

### Why remove `owo-colors`?

Don't get me wrong, `owo-colors` is a great crate, but for our purposes is a bit too heavy. 

Initially, we chose `owo-colors` because of the convenience, and we thought that it might provide Windows support (my bad 😅) and automatic detection of color support.

In 0.3, we have removed automatic detection support and discovered that `owo-colors` does not - in fact - provide Windows support. In that case, it is pretty easy to create an alternative with a similar level of convenience, and after going on an adventure 🥾  with @TimDiekmann through the lands of Windows 11 terminals and discovered that pretty much every built-in terminal either supports ANSI escape sequences natively out-of-the-box, or it can be enabled (on older installations).

### Correctness of `owo-colors`

What about correctness? After looking through snapshots I have discovered that for style attributes like `bold` or `italic` `owo-colors` actually does the wrong thing when resetting the styles. I will be posting a follow-up PR for `owo-colors`, but due to it being passively maintained I think there's a low chance it will be in time for 0.3 anyway.

![image](https://user-images.githubusercontent.com/7252775/215569683-2bf9bd12-7076-4fcc-a7c1-f2c2b3545767.png)

The problem: when using style attributes like `bold` (`\e[1m`) or `italic` (`\e[3m`) when you want to end the portion of text there are two options:
1) nuclear: `\e[0m`, this removes all formatting (`owo-colors` uses this)
2) 🎩 gentleman: `\e[22m` (not `bold`), `\e[23m` (not `italic`), instead of removing and resetting everything it just disables bold or italic respectively, allowing for nested styles and even colored text in italic text (not possible with the nuclear option)

We also compress as much as possible, meaning that `bold italic` is not `\e[1m\e[3m` but instead `\e[1;3m`. Small optimization, but a nice one c:

## 👂 Open Questions

I went ahead and just implemented all ANSI escape sequences, they are currently lacking tests (coming soon), but most are currently unused. Should we remove the ones that are not needed or future-proof the solution?

Do we want to expose the coloring methods through `error-stack`?

> IMO, no, if we want to go ahead with something like that, it should go in a separate crate

## 🔗 Related links

## 📹 Demo
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/libs > error-stack Affects the `error-stack` crate (library)
Development

Successfully merging this pull request may close these issues.

3 participants