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(table): Implement FromIterator for widgets::Row #755

Merged
merged 1 commit into from
Jan 7, 2024

Conversation

Lunderberg
Copy link
Contributor

The Row::new constructor accepts a single argument that implements IntoIterator. This commit adds an implementation of FromIterator, as a thin wrapper around Row::new. This allows .collect::<Row>() to be used at the end of an iterator chain, rather than wrapping the entire iterator chain in Row::new.

Examples in examples/table.rs that used an intermediate variable for the cell iterator have been updated to instead collect directly into a Row. Examples that have an explicit vec![cell1, ..., cellN] have been kept as-is.

Copy link

codecov bot commented Jan 6, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (f13fd73) 92.3% compared to head (7c346f0) 92.3%.
Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##            main    #755     +/-   ##
=======================================
- Coverage   92.3%   92.3%   -0.1%     
=======================================
  Files         55      55             
  Lines      14844   14840      -4     
=======================================
- Hits       13707   13703      -4     
  Misses      1137    1137             

☔ 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.

Neat! The only thing I'd suggest adding would be some docs to the Row type so there's some example to point people in this direction.

Happy to take this as is though if you don't want to spend the time of docs.

Copy link
Member

@Valentin271 Valentin271 left a comment

Choose a reason for hiding this comment

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

I like that too, very good idea!

Oh and your commit needs to be signed for us to merge if that's not too much to ask.

src/widgets/table/row.rs Outdated Show resolved Hide resolved
@joshka
Copy link
Member

joshka commented Jan 6, 2024

Oh and your commit needs to be signed for us to merge if that's not too much to ask.

https://github.com/ratatui-org/ratatui/blob/main/CONTRIBUTING.md#sign-your-commits has some info on this.

@Lunderberg Lunderberg force-pushed the feat_row_fromiterator branch from c2705f4 to 89b3e72 Compare January 7, 2024 02:21
@Lunderberg
Copy link
Contributor Author

The only thing I'd suggest adding would be some docs to the Row type so there's some example to point people in this direction.

Sounds good, and I've updated the docstring for Row to include an example, alongside the Row::new usage.

Oh and your commit needs to be signed for us to merge if that's not too much to ask.

No problem, and it should now be signed.

@Lunderberg Lunderberg force-pushed the feat_row_fromiterator branch from 89b3e72 to b6ba189 Compare January 7, 2024 02:28
The `Row::new` constructor accepts a single argument that implements
`IntoIterator`.  This commit adds an implementation of `FromIterator`,
as a thin wrapper around `Row::new`.  This allows `.collect::<Row>()`
to be used at the end of an iterator chain, rather than wrapping the
entire iterator chain in `Row::new`.

Examples in `examples/table.rs` that used an intermediate variable for
the cell iterator have been updated to instead collect directly into a
`Row`.  Examples that have an explicit `vec![cell1, ..., cellN]` have
been kept as-is.
@Lunderberg Lunderberg force-pushed the feat_row_fromiterator branch from b6ba189 to 7c346f0 Compare January 7, 2024 02:30
@joshka joshka merged commit e64e194 into ratatui:main Jan 7, 2024
33 checks passed
@joshka
Copy link
Member

joshka commented Jan 7, 2024

Thanks for this PR :)

@joshka
Copy link
Member

joshka commented Jan 7, 2024

I wonder if it makes sense to do this for any other types

@Lunderberg Lunderberg deleted the feat_row_fromiterator branch January 7, 2024 19:55
@Lunderberg
Copy link
Contributor Author

I wonder if it makes sense to do this for any other types

I took a brief look at Table when implementing FromIterator for Row, but hadn't looked at it in detail. From a grep for fn new inside the widgets directory, there's two definite cases where it would be useful, and one maybe.

  • Table: Maybe. Prior to fix(table)!: Add widths parameter to new() #664, then impl<'a> FromIterator<Row<'a>> for Table<'a>. However, there isn't a good way for FromIterator to receive the widths argument. It could be called as iter.collect::<Table>().widths(widths), but that would have the possibility of forgetting to call .widths(widths).

  • Tabs: Definitely. The titles: Vec<T> argument is converted to a Vec<Line<'a>>. This argument could be changed to titles: IntoIterator<Item: Into<Line<'a>>, and Tabs could implement FromIterator<T> where T: Into<Line<'a>>.

  • List: Definitely. The List::new method accepts items: IntoIterator<Item: Into<ListItem<'a>>> so there could easily be impl<'a> FromIterator<Item> for List<'a> where Item: Into<ListItem<'a>>.

@joshka
Copy link
Member

joshka commented Jan 7, 2024

Cool. Do you want to raise a couple of issues for these?

Lunderberg added a commit to Lunderberg/ratatui that referenced this pull request Jan 9, 2024
A follow-up from ratatui#755,
allowing any iterator whose item is convertible into `Line` to be
collected into `Tabs`.
Lunderberg added a commit to Lunderberg/ratatui that referenced this pull request Jan 10, 2024
A follow-up from ratatui#755,
allowing any iterator whose item is convertible into `ListItem` to be
collected into a `List`.
Lunderberg added a commit to Lunderberg/ratatui that referenced this pull request Jan 10, 2024
A follow-up from ratatui#755,
allowing any iterator whose item is convertible into `Row` to be
collected into a `Table`.
@Lunderberg
Copy link
Contributor Author

@joshka Sure! I've opened #774, #775, and #776 for the three separate updates. There are some slight variations in how each is implemented, in order to avoid breaking types that may be inferred from Table::new or Tabs::new.

Lunderberg added a commit to Lunderberg/ratatui that referenced this pull request Jan 11, 2024
A follow-up from ratatui#755,
allowing any iterator whose item is convertible into `Line` to be
collected into `Tabs`.

In addition, where previously `Tabs::new` required a `Vec`, it can now
accept any object that implements `IntoIterator` with an item type
implementing `Into<Line>`.
Lunderberg added a commit to Lunderberg/ratatui that referenced this pull request Jan 11, 2024
A follow-up from ratatui#755,
allowing any iterator whose item is convertible into `Line` to be
collected into `Tabs`.

In addition, where previously `Tabs::new` required a `Vec`, it can now
accept any object that implements `IntoIterator` with an item type
implementing `Into<Line>`.

BREAKING CHANGE:

Calls to `Tabs::new()` whose argument is collected from an iterator
will no longer compile.  For example,
`Tabs::new(["a","b"].into_iter().collect())` will no longer compile,
because the return type of `.collect()` can no longer be inferred to
be a `Vec<_>`.
Lunderberg added a commit to Lunderberg/ratatui that referenced this pull request Jan 11, 2024
A follow-up from ratatui#755,
allowing any iterator whose item is convertible into `Line` to be
collected into `Tabs`.

In addition, where previously `Tabs::new` required a `Vec`, it can now
accept any object that implements `IntoIterator` with an item type
implementing `Into<Line>`.

BREAKING CHANGE:

Calls to `Tabs::new()` whose argument is collected from an iterator
will no longer compile.  For example,
`Tabs::new(["a","b"].into_iter().collect())` will no longer compile,
because the return type of `.collect()` can no longer be inferred to
be a `Vec<_>`.
Lunderberg added a commit to Lunderberg/ratatui that referenced this pull request Jan 11, 2024
A follow-up from ratatui#755,
allowing any iterator whose item is convertible into `Row` to be
collected into a `Table`.

Where previously, `Table::new` accepted `IntoIterator<Item = Row>`, it
now accepts `IntoIterator<Item: Into<Row>>`.

BREAKING CHANGE:
The compiler can no longer infer the element type of the container
passed to `Table::new()`.  For example, `Table::new(vec![], widths)`
will no longer compile, as the type of `vec![]` can no longer be
inferred.
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