ratatui-org / ratatui

Rust library that's all about cooking up terminal user interfaces (TUIs) 👨‍🍳🐀
https://ratatui.rs
MIT License
8.86k stars 269 forks source link

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

Closed matta closed 1 month ago

matta commented 1 month ago

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.

matta commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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)

codecov[bot] commented 1 month ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

matta commented 1 month ago

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.