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

docs(examples): Remove lifetimes from the List example #1132

Merged
merged 2 commits into from
May 24, 2024

Conversation

matta
Copy link
Contributor

@matta matta commented May 24, 2024

Simplify the List example by removing lifetimes not strictly necessary to demonstrate how Ratatui lists work. Instead, the sample strings are copied into each TodoItem. To further simplify, I changed the code to use a new TodoItem::new function, rather than an implementation of the From trait.

Lifetimes are one of the more confusing things people learning Rust have to understand. The goal here is to remove that concern from the example. The From trait, also, is something a person new to Rust may not fully understand.

Simplify the List example by removing lifetimes not strictly necessary
to demonstrate how Ratatui lists work. Instead, the sample strings are
copied into each `TodoItem`. To further simplify, I changed the code to
use a new TodoItem::new function, rather than an implementation of the
`From` trait.

Lifetimes are one of the more confusing things people learning Rust
have to understand. The goal here is to remove that concern from
the example. The `From` trait, also, is something a person new to
Rust may not fully understand.
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.

LGTM. I like the simplification of this. It also makes a lot more sense for the StatefulList to own the strings rather than reference them from strings that aren't really stored anywhere)

What about also:

  • rename StatefulList to TodoList
  • move the code that converts from Vec to ListItem into a method on TodoList (this really should be impl From<TodoList> for Vec<ListItem> and not a bare method).
  • Adding a few small comments where they help

Right now TodoItem -> ListItem can't just be From for ListItem as it needs to know the item index for row coloring. I think the example should show how to convert into the ListItem using From, as this . Do you think it would be worth moving the index into the TodoItem to make that possible?

Approving as there's no need to make the changes mentioned above (they're nice to have though if you are inclined to improve this further).

@matta
Copy link
Contributor Author

matta commented May 24, 2024

move the code that converts from Vec to ListItem into a method on TodoList (this really should be impl From for Vec and not a bare method).

Help me understand this. Is impl From<TodoList> for Vec<ListItem> considered a method on TodoList? I sort-of view the From and Into traits as great for expressing conversions between value types that are expected to be used in many places and commonly converted to and fro. For bespoke conversions, perhaps a named method is better? Outside of an IDE it isn't always clear what is happening when reading .into() in code.

@joshka
Copy link
Member

joshka commented May 24, 2024

move the code that converts from Vec to ListItem into a method on TodoList (this really should be impl From for Vec and not a bare method).

Help me understand this. Is impl From<TodoList> for Vec<ListItem> considered a method on TodoList? I sort-of view the From and Into traits as great for expressing conversions between value types that are expected to be used in many places and commonly converted to and fro. For bespoke conversions, perhaps a named method is better? Outside of an IDE it isn't always clear what is happening when reading .into() in code.

It's a trait implementation. It's similar but not the same. If you look at what List::new() accepts, you'll see that it accepts Iterator<Item = Into<ListItem>>, which means if you implement the appropriate conversion traits then you can just create the list using e.g. List::new(self.todo_list). You shouldn't have to write .into() at all here. I'd say that implementing a conversion trait is the idiomatic and correct way to write code that does a conversion like this (I'd generally refactor towards it in pretty much any situation where it makes sense). I'm surprised there's not a "This looks like a conversion, implement From" clippy lint.

@EdJoPaTo
Copy link
Contributor

EdJoPaTo commented May 24, 2024

Personal thought on when to use From/Into and when not: When its a simple thing into something else (= no clones, pure moves / copy) and is rather simple / discoverable. When it involves pairs for example it's almost always easier to understand with names on functions (or putting into a struct).

From/Into is hard to understand without IDE and with tooling its still somewhat annyoing to see what exactly is done. It can also be annoying on refactorings as it silently changes the implementation which might do unexpected stuff (especially annoying when they involve cloning or performance critical things -> dont implement stuff like that in From/into)

Copy link

codecov bot commented May 24, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 94.3%. Comparing base (257db62) to head (99d268d).
Report is 2 commits behind head on main.

Additional details and impacted files
@@          Coverage Diff          @@
##            main   #1132   +/-   ##
=====================================
  Coverage   94.3%   94.3%           
=====================================
  Files         61      60    -1     
  Lines      14767   14757   -10     
=====================================
- Hits       13926   13917    -9     
+ Misses       841     840    -1     

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

@matta
Copy link
Contributor Author

matta commented May 24, 2024

I went ahead and renamed StatefulList to TodoList. Feel free to squash commit as you see fit!

From/Into is hard to understand without IDE and with tooling its still somewhat annyoing to see what exactly is done. It can also be annoying on refactorings as it silently changes the implementation which might do unexpected stuff (especially annoying when they involve cloning or performance critical things -> dont implement stuff like that in From/into)

Yes, this was my thinking too. In this case, allocating a temporary vector with potentially no visual artifact in code where this allocation might be occurring is arguably confusing and not very desirable, in my opinion.

I did experiment with adding a method to TodoList that returned impl Iterator<Item=ListItem> + '_ but this ran into borrow check issues later on with the call to StatefulWidget::render. I think it is best left alone, or a considered in a separate PR.

@joshka joshka merged commit f429f68 into ratatui:main May 24, 2024
33 checks passed
joshka pushed a commit to erak/ratatui that referenced this pull request Oct 14, 2024
Simplify the List example by removing lifetimes not strictly necessary
to demonstrate how Ratatui lists work. Instead, the sample strings are
copied into each `TodoItem`. To further simplify, I changed the code to
use a new TodoItem::new function, rather than an implementation of the
`From` trait.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants