Clean up where Self: Sized | No breaking library changes#215
Clean up where Self: Sized | No breaking library changes#215spenserblack merged 5 commits intocolored-rs:masterfrom
where Self: Sized | No breaking library changes#215Conversation
|
Mods, please merge this immediately. I am really looking forward to this great feature. Can't wait!!! |
There was a problem hiding this comment.
Pull request overview
This PR refactors the Colorize trait to improve code cleanliness by removing repetitive where Self: Sized constraints and applying various code quality improvements. The changes include adding Sized as a trait bound on Colorize itself, making internal functions const, improving variable naming, and fixing formatting issues.
- Added
Sizedconstraint to theColorizetrait definition, eliminating the need for per-methodwhereclauses - Made internal helper functions
constwhile keeping public API non-const - Renamed
has_wrotetohas_writtenfor correct grammar and improved various code formatting issues
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/lib.rs | Refactored Colorize trait with Sized bound, removed per-method where clauses, renamed has_wrote to has_written, simplified generic constraints, and added spacing between struct fields |
| src/style.rs | Made to_str() and to_u8() functions const, removed unnecessary parentheses in filter closure, and added whitespace for readability |
| src/color.rs | Made into_truecolor() function const and added blank line for formatting |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/lib.rs
Outdated
| self.on_color(Color::AnsiColor(color.into())) | ||
| } | ||
| fn on_color<S: Into<Color>>(self, color: S) -> ColoredString; | ||
| fn on_color<T: Into<Color>>(self, color: T) -> ColoredString; |
There was a problem hiding this comment.
The generic parameter was renamed from S to T, creating inconsistency with the color method on line 216 which still uses S. Both methods should use the same generic parameter name for consistency.
| fn on_color<T: Into<Color>>(self, color: T) -> ColoredString; | |
| fn on_color<S: Into<Color>>(self, color: S) -> ColoredString; |
spenserblack
left a comment
There was a problem hiding this comment.
Looks all good to me! Thanks! Just this small nitpick. What do you think?
|
https://github.com/colored-rs/colored/actions/runs/20279679954/job/58238026056 says that this requires a new version, but, given that it's impossible to implement the trait on an unsized type AFAIK this is not in fact breaking IMO. |
I mostly changed the clunky where-clauses in the
Colorizetrait.Before:
After:
For this to work, I added the
Sizedconstraint in the trait itself:This has no caveats: The
selfparameter is passed in all methods, so the "globalization" of the constraint has no downsides.The rest of the changes were carefully applied lints: I added
constto some internal functions, but made sure to keep public functions non-const to not potentially force breaking API changes later. Also,has_wroteis not proper English lol.If there is room for improvement or I made some mistake, please let me know.