Support for custom styles

[d4h0]

Hi,

This implements support for custom styles (required for #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]

@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]

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]

@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]

This looks great, thank you so much for putting in the time to set this all up. I'm not 100% done with the review but I figured I'd post the comments I have so far. I'm going to try to fix the merge conflict now and then I'll go ahead and check into the comments around STYLES.get() and then finally if I have time before the error handling project group meeting in 20 minutes I'll take a look at the color-spantrace PR.

[yaahc]

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]

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]

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]

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]

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

[yaahc]

I think this looks good, pending review of how the docs look and writing a changelog.

[yaahc]

this is sick

[yaahc]

ty again for all of this amazing work @d4h0. release 0.5.10 is now out and has your changes in it. ✨ 🎉 🚀

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]

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]

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]

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