search

eyre-rs / color-eyre

Custom hooks for colorful human oriented error reports via panics and the eyre crate
Other
961 stars 56 forks source link

Support for custom styles #69

Closed d4h0 closed 3 years ago

d4h0 commented 4 years ago

Hi,

This implements support for custom styles (required for https://github.com/yaahc/color-eyre/issues/58).

There are a few comments with questions for you, which I have marked with "XXX" (so they are easy to find with grep or ripgrep).

Please let me know if there is any problem or if something could improve. If you want to rename something (English is not my native language), just tell me to do so! :)

The test data are not included at the moment, in case you want to tweak the styles a bit (you probably want, see my "XXX" comments).

After you are ready, you just run the tests in "tests/styles.rs" and follow the instructions to generate the test control data.

d4h0 commented 4 years ago

@yaahc: So there is a merge conflict.

These PRs, basically, are the first time I used Git for something more complex than "pull/commit/push", so I never had a merge conflict. Do I need to do anything, or will you fix this?

d4h0 commented 4 years ago

As with the other PR, the test suite will fail because I didn't include the test data.

I will generate and include them now.

Regarding the merge conflict: I looked at the conflict and it looks very simple. But I don't want to resolve it myself (in case I'm missing something and will break something).

yaahc commented 4 years ago

@yaahc: So there is a merge conflict.

These PRs, basically, are the first time I used Git for something more complex than "pull/commit/push", so I never had a merge conflict. Do I need to do anything, or will you fix this?

I can fix the merge conflict, no worries

yaahc commented 4 years ago

Okay I went ahead and fixed the merge conflict but It looks like the owo-colors stuff doesn't seem to compile, I'm guessing because it still depends on an unreleased version, so I may have introduced some errors. Once the owo-colors changes are released to crates make sure to update the Cargo.toml to pin the correct minimum version.

d4h0 commented 4 years ago

Thanks, for the feedback, @yaahc!

In the coming few days I should have enough time to work through your suggestions here and at color_spantrace.

d4h0 commented 4 years ago

Alright, I believe I have implemented all suggestions. Let me know if I missed anything!

I realized I mixed up color-backtrace with color-spantrace (we had InstallColorBacktraceStylesError).

Please keep an eye open for mistakes like these at the public API level... 😅

As with color_spantrace, one test fails because I have removed the xterm colors from the default theme (in case you want to compare 'new' and 'old'). I'll generate new test data when a new owo_colors version is released.

I have a few additional questions:

1) jam1garner offered to release a new version if we want to publish this earlier (otherwise, it could take some time, until the next version is released).

Should we ask for this?

Personally, I don't care much, but maybe there will be new merge conflicts in the future, otherwise.

If not, and you'd like to properly test my PRs, you could patch your version of owo_colors with the current master (my PR is already merged).

2) Is there a better way to add notes, than to add "XXX" comments? I'm not sure if this is the best way to request feedback.

3) examples/theme_test_helper.rs could be a bin with optional dependencies. Would that make more sense than an example?

Personally, I think this could confuse people who look at Cargo.toml (and it would be more "invasive" in regard to Cargo.toml).

yaahc commented 4 years ago

I'm sorry for not replying to this for so long, life has been getting in the way. Just know that this is high on my list of priorities and I'll get to this as soon as I can.

d4h0 commented 4 years ago

Don't worry, I was assuming exactly this, and I have enough other stuff to do :)

yaahc commented 3 years ago

ty again for all of this amazing work @d4h0. release 0.5.10 is now out and has your changes in it. :sparkles: :tada: :rocket:

And just once again, I'm really sorry about how long I took on this. I know you understand but still, ty for handling the delay so graciously.

d4h0 commented 3 years ago

Awesome – thank you so much, @yaahc! 😁

I've added two comments, but I guess you got notifications for them.

I hope it wasn't too difficult to finish this. If there is anything I could do better when contributing to a project, please let me know! (I guess, at the very least, I need to learn Git properly... 😅)

yaahc commented 3 years ago

I hope it wasn't too difficult to finish this. If there is anything I could do better when contributing to a project, please let me know! (I guess, at the very least, I need to learn Git properly... sweat_smile)

You did fantastic work, and no it wasn't difficult. The test took some thinking to figure out how to get it to work on all our CI configurations but it ended up being simple and working great with all the helper code you setup.

d4h0 commented 3 years ago

You did fantastic work, and no it wasn't difficult.

Awesome, thanks! 😄

The test took some thinking to figure out how to get it to work on all our CI configurations but it ended up being simple and working great with all the helper code you setup.

I think I had similar problems while creating the test (e.g. because of my workspace configuration). Hopefully, this kind of problem will not happen too often... 😅