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

refactor!: make patch_style & reset_style chainable #754

Merged

Conversation

Valentin271
Copy link
Member

Previously, patch_style and reset_style in Text, Line and Span were using a mutable reference to Self. To be more consistent with the rest of ratatui, which is using fluent setters, these now take ownership of Self and return it.

Closes #644

Copy link

codecov bot commented Jan 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (bc274e2) 92.3% compared to head (da5af28) 92.3%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #754     +/-   ##
=======================================
- Coverage   92.3%   92.3%   -0.1%     
=======================================
  Files         55      55             
  Lines      14831   14828      -3     
=======================================
- Hits       13694   13691      -3     
  Misses      1137    1137             

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

@kdheepak
Copy link
Collaborator

kdheepak commented Jan 6, 2024

I use .patch_style on a field in struct:

struct Config {
    calendar: Style
}

fn user_config() {
  // ...
  config.calendar.patch_style(Style::default().fg(color_from_user_input))
  // ...
}

I wouldn't be able to do this though if .patch_style takes an owned value.

@joshka
Copy link
Member

joshka commented Jan 6, 2024

calendar: Style

I wouldn't be able to do this though if .patch_style takes an owned value.

This doesn't change Style.patch_style, which already takes an owned value for the style parameter. Style implements Copy, so it doesn't really matter if it's owned.

I think maybe you're talking about not being able to do this on Lines/Spans that are stored?

BREAKING-CHANGES.md Outdated Show resolved Hide resolved
@Valentin271 Valentin271 force-pushed the refactor/patch_style-chainable branch from e6bb57c to 278adbe Compare January 6, 2024 21:15
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.

Approved, but I want to make sure that @kdheepak's concern is addressed before merging.

src/text/line.rs Outdated Show resolved Hide resolved
@Valentin271 Valentin271 force-pushed the refactor/patch_style-chainable branch 4 times, most recently from 6e56522 to ab928fb Compare January 6, 2024 23:19
@Valentin271
Copy link
Member Author

Kind of messed up the rebase but its up to date now.

@kdheepak
Copy link
Collaborator

kdheepak commented Jan 7, 2024

My bad, didn't review the code changes properly. I don't store Span in a struct, so this wouldn't be a problem for me. I'm assuming storing Span and then patching it is uncommon.

@joshka
Copy link
Member

joshka commented Jan 7, 2024

@Valentin271 the doc tests are failing.

Previously, `patch_style` and `reset_style` in `Text`, `Line` and `Span`
 were using a mutable reference to `Self`. To be more consistent with
 the rest of `ratatui`, which is using fluent setters, these now take
 ownership of `Self` and return it.
@Valentin271 Valentin271 force-pushed the refactor/patch_style-chainable branch from ab928fb to da5af28 Compare January 7, 2024 11:20
@Valentin271
Copy link
Member Author

I forgot to update the tests after the Line update.

I'd like another review on this because I modified the examples to be more concise and better demonstrate the effect. Before that the examples were the exact same as the tests.

@Valentin271 Valentin271 merged commit bd6b91c into ratatui-org:main Jan 7, 2024
33 checks passed
@Valentin271 Valentin271 deleted the refactor/patch_style-chainable branch January 7, 2024 11:58
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.

.patch_style() methods should be chainable (BREAKING CHANGE)
4 participants