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

fix(widgets)!: rename StatefulWidgetRef::render_ref to render_stateful_ref #1184

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

joshka
Copy link
Member

@joshka joshka commented Jun 17, 2024

This commit renames the StatefulWidgetRef::render_ref method to
render_stateful_ref. This helps avoid collisions with the WidgetRef
trait's render_ref method. This change is breaking and requires
updating all implementations of StatefulWidgetRef.

BREAKING CHANGE: StatefulWidgetRef::render_ref has been renamed to
StatefulWidgetRef::render_stateful_ref.

 trait StatefulWidgetRef {
     type State;
-    fn render_ref(&self, area: Rect, buf: &mut Buffer, state: &mut Self::State) { }
+    fn render_stateful_ref(&self, area: Rect, buf: &mut Buffer, state: &mut Self::State) { }
 }

Partially addresses #996

…ul_ref`

This commit renames the `StatefulWidgetRef::render_ref` method to
`render_stateful_ref`. This helps avoid collisions with the `WidgetRef`
trait's `render_ref` method. This change is breaking and requires
updating all implementations of `StatefulWidgetRef`.

BREAKING CHANGE: `StatefulWidgetRef::render_ref` has been renamed to
`StatefulWidgetRef::render_stateful_ref`.

```diff
 trait StatefulWidgetRef {
     type State;
-    fn render_ref(&self, area: Rect, buf: &mut Buffer, state: &mut Self::State) { }
+    fn render_stateful_ref(&self, area: Rect, buf: &mut Buffer, state: &mut Self::State) { }
 }
```

Partially addresses <#996>
@github-actions github-actions bot added the Type: Breaking Change This change will cause application code to break and must be noted in the breaking changes docs etc. label Jun 17, 2024
@joshka joshka changed the title fix(widgets)!: rename StatefulWidgetRef::render_ref to render_stateful_ref fix(widgets)!: rename StatefulWidgetRef::render_ref to render_stateful_ref Jun 17, 2024
Copy link

codecov bot commented Jun 17, 2024

Codecov Report

Attention: Patch coverage is 87.50000% with 1 line in your changes missing coverage. Please review.

Project coverage is 94.3%. Comparing base (4bfdc15) to head (330d071).
Report is 6 commits behind head on main.

Files Patch % Lines
src/terminal/frame.rs 0.0% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1184   +/-   ##
=====================================
  Coverage   94.3%   94.3%           
=====================================
  Files         60      60           
  Lines      14679   14679           
=====================================
  Hits       13843   13843           
  Misses       836     836           

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

@kdheepak
Copy link
Collaborator

kdheepak commented Jun 17, 2024

I think we could do render_ref_with_state() instead, but I'm fine with render_stateful_ref(). Maybe check in with other maintainers before merging?

@joshka
Copy link
Member Author

joshka commented Jun 17, 2024

I think we could do render_ref_with_state() instead, but I'm fine with render_stateful_ref(). Maybe check in with other maintainers before merging?

I thought about that a little prior to writing this. The main pro for _stateful is that it's in the name of the trait already, so it's kinda expected to be in the method name. The main pro for _with_state seems is that it's a natural name for what this does. For me this feels like a 55/45 naming decision. I have a small amount of preference for _stateful, but would be convinced if there were good examples of _with_xxx on trait implementations in std.

@kdheepak
Copy link
Collaborator

I have a preference for renaming these new trait names (the ones that are unstable) to Render and RenderWithState and then changing the names of the trait methods to match accordingly.

The disadvantage is that it would break from convention with the Widget and StatefulWidget traits. The advantage is that it is more consistent and clear (imo).

And I can see a world where everyone moves to using these new traits, and we deprecate the old ones and remove them eventually (a few years later?).

@joshka
Copy link
Member Author

joshka commented Jun 17, 2024

I have a preference for renaming these new trait names (the ones that are unstable) to Render and RenderWithState and then changing the names of the trait methods to match accordingly.

The disadvantage is that it would break from convention with the Widget and StatefulWidget traits. The advantage is that it is more consistent and clear (imo).

And I can see a world where everyone moves to using these new traits, and we deprecate the old ones and remove them eventually (a few years later?).

Let's move this point to a discussion on the forum.

@joshka
Copy link
Member Author

joshka commented Jun 19, 2024

Let's move this point to a discussion on the forum.

https://forum.ratatui.rs/t/naming-render-traits-methods/68

@kdheepak based on the logic in the above forum post are you happy to see this merged as is, or would you prefer to hold for a bit on this. It would be good to get it out in 0.27.0 perhaps.

@joshka joshka added Status: Pending Waiting on clarification / more information from submitter Status: Controversial There is some discussion about whether this is the right direction. Clarify and seek consensus. labels Jun 19, 2024
@EdJoPaTo
Copy link
Member

Probably off-topic to this PR… why move this to a different place? Personally the forum feels like a place for "how to do this" and "showoffs". Stuff that should end in the code should be discussed as issues next to the code.

To the topic of naming the trait / methods… Personally I went to ratatui::widget::TRAIT::render(widget, …) explicitly and that works well so far for me without naming conflicts. This is not ideal, but until there is a good way of handling this trait thing, good enough. Somehow this feels a bit rushed and all over the place to me currently. Discussion in multiple PR, the forum, …. This PR seems more like a draft to me currently. This might be a way to go forward. But it's nothing that should be merged for the next release as it will result in a lot of churn for sure.

@kdheepak
Copy link
Collaborator

I’d like to hold out till we figure out the naming, if that’s okay. Making one breaking change later (v0.28.0?) is better than making one now and potentially again one later.

@joshka joshka added the Status: On Hold Not actively being worked on for now due to time / other constraints label Jun 19, 2024
@joshka
Copy link
Member Author

joshka commented Jun 20, 2024

Probably off-topic to this PR… why move this to a different place? Personally the forum feels like a place for "how to do this" and "showoffs". Stuff that should end in the code should be discussed as issues next to the code.

It is off-topic for this PR. If you ask about the rationale for this on the forum, I'll answer there... ;P

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Status: Controversial There is some discussion about whether this is the right direction. Clarify and seek consensus. Status: On Hold Not actively being worked on for now due to time / other constraints Status: Pending Waiting on clarification / more information from submitter Type: Breaking Change This change will cause application code to break and must be noted in the breaking changes docs etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants