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

feat: Make Stylize's .bg(color) generic #1099

Merged
merged 5 commits into from
May 12, 2024
Merged

feat: Make Stylize's .bg(color) generic #1099

merged 5 commits into from
May 12, 2024

Conversation

kdheepak
Copy link
Collaborator

This PR makes .bg(color) generic accepting anything that can be converted into Color; similar to the .fg(color) method on the same trait

Copy link

codecov bot commented May 12, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.2%. Comparing base (699c2d7) to head (d098ec6).

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1099   +/-   ##
=====================================
  Coverage   94.2%   94.2%           
=====================================
  Files         61      61           
  Lines      14530   14530           
=====================================
  Hits       13693   13693           
  Misses       837     837           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@joshka joshka left a comment

Choose a reason for hiding this comment

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

Does this need any updated tests?

Copy link
Member

@EdJoPaTo EdJoPaTo left a comment

Choose a reason for hiding this comment

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

As every impl needs to change this is a breaking change

src/style/stylize.rs Outdated Show resolved Hide resolved
@kdheepak
Copy link
Collaborator Author

Does this need any updated tests?

There weren't any tests for this. I think there's no implementations for From objects into Color in Ratatui. There's crossterm::style::Color -> ratatui::style::Color but that test will only work for the crossterm backend.

As every impl needs to change this is a breaking change

This is strictly breaking but in practice I don't think it'll break code because Stylize is fairly new and people may have not had the chance or reason to implement it. In the past we've merged similar "strictly breaking but not breaking in practice" changes in patch releases. Thoughts?

Open to suggestions on making this protected.

@joshka
Copy link
Member

joshka commented May 12, 2024

Open to suggestions on making this protected.

No hard opinion on this, but I know that I hate seeing traits in other libs that I'd like to be able to implement even if they later change them in the lib, but can't because they're not exposed, so I'd lean towards just keeping it as is, and dealing with breaking changes in the usual manners (communicating when we can, being mindful of the changes, etc.)

@kdheepak kdheepak merged commit ec763af into main May 12, 2024
33 checks passed
@kdheepak kdheepak deleted the kd/stylize-bg branch May 12, 2024 17:23
@@ -47,6 +49,14 @@ This is a quick summary of the sections below:
- MSRV is now 1.63.0
- `List` no longer ignores empty strings

## [v0.26.1](https://github.com/ratatui-org/ratatui/releases/tag/v0.26.1)
Copy link
Member

Choose a reason for hiding this comment

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

Is this the correct version for this - likely 'unreleased' or 0.26.3

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ugh my bad.

@joshka
Copy link
Member

joshka commented May 12, 2024

Also - 0.26.3 was slated to be a bugfix / no breaking changes release tomorrow, to make it easy for broken apps to just be fixed without any changes required. (version spec 0.26.2 will actually use 0.26.3 automatically when resolving, but it won't use 0.27.0)

@kdheepak
Copy link
Collaborator Author

Should I revert this PR until 0.27.0?

kdheepak added a commit that referenced this pull request May 12, 2024
kdheepak added a commit that referenced this pull request May 12, 2024
joshka pushed a commit to nowNick/ratatui that referenced this pull request May 24, 2024
This PR makes `.bg(color)` generic accepting anything that can be
converted into `Color`; similar to the `.fg(color)` method on the same
trait
joshka pushed a commit to nowNick/ratatui that referenced this pull request May 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants