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(List)!: new now accepts IntoIterator<Item = Into<ListItem>> #672

Merged
merged 1 commit into from
Dec 8, 2023

Conversation

Valentin271
Copy link
Member

This allows to build list like

List::new(["Item 1", "Item 2"])

Fixes #670

I think this should be added for 0.26 to not add too much breaking change for 0.25. Moreover there's another breaking change for list coming with #671. It makes sense to release them together.

@Valentin271 Valentin271 added Type: Breaking Change This change will cause application code to break and must be noted in the breaking changes docs etc. Status: Do Not Merge Prevent a PR from being merged. Usually indicates something that should be held to a future release labels Dec 7, 2023
@Valentin271 Valentin271 added this to the v0.26.0 milestone Dec 7, 2023
Copy link

codecov bot commented Dec 7, 2023

Codecov Report

Attention: 9 lines in your changes are missing coverage. Please review.

Comparison is base (8bfd666) 90.9% compared to head (511c86d) 90.9%.

Files Patch % Lines
src/widgets/list.rs 85.9% 9 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##            main    #672     +/-   ##
=======================================
- Coverage   90.9%   90.9%   -0.1%     
=======================================
  Files         42      42             
  Lines      12612   12671     +59     
=======================================
+ Hits       11469   11519     +50     
- Misses      1143    1152      +9     

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

I think this breaks things in a good way (i.e. on an empty list of items with indeterminate type), so we can probably take it in the next major (0.25.0).

Needs a couple more things:

  • some tests to show it works for a few of the assorted types.
  • an entry in the breaking changes doc to explain how old code will fail (and corresponding changelog message in the commit

Let's also add an items() method so that List::default().items(...) works. What do you think?

src/widgets/list.rs Show resolved Hide resolved
@Valentin271
Copy link
Member Author

Let's go for 0.25 then, it shouldn't really break any code anyway since there weren't an items method before. So creating an empty list was pointless.

Let's also add an items() method so that List::default().items(...) works. What do you think?

Agree, so it lines up with table once #638 is merged

@Valentin271 Valentin271 modified the milestones: v0.26.0, v0.25.0 Dec 8, 2023
@Valentin271
Copy link
Member Author

Should be all good now. Hope the commit message is good enough, took inspiration from this one 37c70db

@Valentin271 Valentin271 removed the Status: Do Not Merge Prevent a PR from being merged. Usually indicates something that should be held to a future release label Dec 8, 2023
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 - the must_use fix should probably be added before merging this though.

src/widgets/list.rs Show resolved Hide resolved
tests/widgets_list.rs Outdated Show resolved Hide resolved
…em>>`

This allows to build list like

```
List::new(["Item 1", "Item 2"])
```

BREAKING CHANGE: `List::new` parameter type changed from `Into<Vec<ListItem<'a>>>`
to `IntoIterator<Item = Into<ListItem<'a>>>`
@Valentin271
Copy link
Member Author

I also fixed some typos and missing links in the breaking-change file. Weird that typos-cli didn't catch them.

@joshka joshka merged commit aef4956 into ratatui:main Dec 8, 2023
32 of 33 checks passed
@Valentin271 Valentin271 deleted the feat/list-new-intoiterator branch December 9, 2023 09:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

Make List::new accept IntoIterator
3 participants