Skip to content

Add Clone trait to Unique#777

Merged
Philippe-Cholet merged 1 commit intorust-itertools:masterfrom
gilhooleyd:unique_add_clone
Oct 8, 2023
Merged

Add Clone trait to Unique#777
Philippe-Cholet merged 1 commit intorust-itertools:masterfrom
gilhooleyd:unique_add_clone

Conversation

@gilhooleyd
Copy link
Copy Markdown
Contributor

@gilhooleyd gilhooleyd commented Oct 6, 2023

Using a Unique iterator requires the Clone trait, so require this trait when creating the Unique object.

See the discussion in #776

@Philippe-Cholet
Copy link
Copy Markdown
Member

Philippe-Cholet commented Oct 6, 2023

I was not gonna suggest to make a test for it but I see your point.
I would only suggest to keep things as simple as possible with just #[...] struct Item(u32);.
And format to pass CI.

@Philippe-Cholet
Copy link
Copy Markdown
Member

Philippe-Cholet commented Oct 6, 2023 •
edited
Loading

Is the unique test not enough?! (in the same test file)

@jswrenn
Copy link
Copy Markdown
Member

jswrenn commented Oct 6, 2023

Yeah, there's no need to add an extra test here. Delete the test, run cargo fmt, and we can merge this.

@gilhooleyd
Copy link
Copy Markdown
Contributor Author

gilhooleyd commented Oct 6, 2023

Thanks for both of your help! I think I've removed the test with the new CL.

I added the test originally when I was trying to remove Clone, and the struct I created didn't implement Clone.
You're right that now that Clone is required, this does the exact same thing as the existing tests.

I wish this functionality didn't require Clone, but I see now that it's necessary to do the in-place iteration while removing dups. Thank you again for your time!

@jswrenn
Copy link
Copy Markdown
Member

jswrenn commented Oct 6, 2023

Ah, wait, could you also add it to the struct definition?

@jswrenn
Copy link
Copy Markdown
Member

jswrenn commented Oct 6, 2023 •
edited by Philippe-Cholet
Loading

Like so:

#[derive(Clone)]
#[must_use = "iterator adaptors are lazy and do nothing unless consumed"]
pub struct Unique<I>
where
    I: Iterator,
    I::Item: Eq + Hash + Clone,
{
    iter: UniqueBy<I, I::Item, ()>,
}

@gilhooleyd
Copy link
Copy Markdown
Contributor Author

gilhooleyd commented Oct 6, 2023

Done! Apologies for the churn

@Philippe-Cholet
Copy link
Copy Markdown
Member

Philippe-Cholet commented Oct 7, 2023 •
edited
Loading

It's not a problem but could you squash these commits into one simple commit?

EDIT: Done, thanks!

@gilhooleyd @Philippe-Cholet
Add Clone trait to Unique
328f956 
Using a Unique iterator requires the Clone trait, so
require this trait when creating the Unique object.
@Philippe-Cholet Philippe-Cholet added this pull request to the merge queue Oct 8, 2023
Merged via the queue into rust-itertools:master with commit 87b41a5 Oct 8, 2023
@jswrenn jswrenn mentioned this pull request Nov 14, 2023
Merged
@jswrenn jswrenn added this to the next milestone Nov 14, 2023
facebook-github-bot pushed a commit to facebookincubator/reindeer that referenced this pull request Oct 14, 2024
@ship-it-ship-it @facebook-github-bot
update itertools 0.11.0 -> 0.12.1
ce89ede 
Summary:
[Release Notes for 0.12.0](https://github.com/rust-itertools/itertools/blob/master/CHANGELOG.md#0120)
[Release Notes for 0.12.1](https://github.com/rust-itertools/itertools/blob/master/CHANGELOG.md#0121)

### Breaking
- Made `take_while_inclusive` consume iterator by value ([#709](rust-itertools/itertools#709)) --- there are [2 usages](https://www.internalfb.com/code/search?q=repo%3Afbcode%20take_while_inclusive&lang_filter=rust) in fbcode, verified both manually
- Added `Clone` bound to `Unique` ([#777](rust-itertools/itertools#777)) --- there are [37 usages](https://fburl.com/code/hp7vdlch) in fbcode, CI will tell if it breaks

Reviewed By: anps77

Differential Revision: D64305791

fbshipit-source-id: fe99131b206905133c4d2b75693090f5ce44f4ca
facebook-github-bot pushed a commit to facebookexperimental/rust-shed that referenced this pull request Oct 14, 2024
@ship-it-ship-it @facebook-github-bot
update itertools 0.11.0 -> 0.12.1
772a9b8 
Summary:
[Release Notes for 0.12.0](https://github.com/rust-itertools/itertools/blob/master/CHANGELOG.md#0120)
[Release Notes for 0.12.1](https://github.com/rust-itertools/itertools/blob/master/CHANGELOG.md#0121)

### Breaking
- Made `take_while_inclusive` consume iterator by value ([#709](rust-itertools/itertools#709)) --- there are [2 usages](https://www.internalfb.com/code/search?q=repo%3Afbcode%20take_while_inclusive&lang_filter=rust) in fbcode, verified both manually
- Added `Clone` bound to `Unique` ([#777](rust-itertools/itertools#777)) --- there are [37 usages](https://fburl.com/code/hp7vdlch) in fbcode, CI will tell if it breaks

Reviewed By: anps77

Differential Revision: D64305791

fbshipit-source-id: fe99131b206905133c4d2b75693090f5ce44f4ca
facebook-github-bot pushed a commit to facebook/sapling that referenced this pull request Oct 14, 2024
@ship-it-ship-it @facebook-github-bot
update itertools 0.11.0 -> 0.12.1
5fbcfb1 
Summary:
[Release Notes for 0.12.0](https://github.com/rust-itertools/itertools/blob/master/CHANGELOG.md#0120)
[Release Notes for 0.12.1](https://github.com/rust-itertools/itertools/blob/master/CHANGELOG.md#0121)

### Breaking
- Made `take_while_inclusive` consume iterator by value ([#709](rust-itertools/itertools#709)) --- there are [2 usages](https://www.internalfb.com/code/search?q=repo%3Afbcode%20take_while_inclusive&lang_filter=rust) in fbcode, verified both manually
- Added `Clone` bound to `Unique` ([#777](rust-itertools/itertools#777)) --- there are [37 usages](https://fburl.com/code/hp7vdlch) in fbcode, CI will tell if it breaks

Reviewed By: anps77

Differential Revision: D64305791

fbshipit-source-id: fe99131b206905133c4d2b75693090f5ce44f4ca
facebook-github-bot pushed a commit to facebook/errpy that referenced this pull request Oct 14, 2024
@ship-it-ship-it @facebook-github-bot
update itertools 0.11.0 -> 0.12.1
7080a2f 
Summary:
[Release Notes for 0.12.0](https://github.com/rust-itertools/itertools/blob/master/CHANGELOG.md#0120)
[Release Notes for 0.12.1](https://github.com/rust-itertools/itertools/blob/master/CHANGELOG.md#0121)

### Breaking
- Made `take_while_inclusive` consume iterator by value ([#709](rust-itertools/itertools#709)) --- there are [2 usages](https://www.internalfb.com/code/search?q=repo%3Afbcode%20take_while_inclusive&lang_filter=rust) in fbcode, verified both manually
- Added `Clone` bound to `Unique` ([#777](rust-itertools/itertools#777)) --- there are [37 usages](https://fburl.com/code/hp7vdlch) in fbcode, CI will tell if it breaks

Reviewed By: anps77

Differential Revision: D64305791

fbshipit-source-id: fe99131b206905133c4d2b75693090f5ce44f4ca
facebook-github-bot pushed a commit to facebook/hhvm that referenced this pull request Oct 14, 2024
@ship-it-ship-it @facebook-github-bot
update itertools 0.11.0 -> 0.12.1
e39efde 
Summary:
[Release Notes for 0.12.0](https://github.com/rust-itertools/itertools/blob/master/CHANGELOG.md#0120)
[Release Notes for 0.12.1](https://github.com/rust-itertools/itertools/blob/master/CHANGELOG.md#0121)

### Breaking
- Made `take_while_inclusive` consume iterator by value ([#709](rust-itertools/itertools#709)) --- there are [2 usages](https://www.internalfb.com/code/search?q=repo%3Afbcode%20take_while_inclusive&lang_filter=rust) in fbcode, verified both manually
- Added `Clone` bound to `Unique` ([#777](rust-itertools/itertools#777)) --- there are [37 usages](https://fburl.com/code/hp7vdlch) in fbcode, CI will tell if it breaks

Reviewed By: anps77

Differential Revision: D64305791

fbshipit-source-id: fe99131b206905133c4d2b75693090f5ce44f4ca
facebook-github-bot pushed a commit to facebookincubator/below that referenced this pull request Oct 14, 2024
@ship-it-ship-it @facebook-github-bot
update itertools 0.11.0 -> 0.12.1
475075f 
Summary:
[Release Notes for 0.12.0](https://github.com/rust-itertools/itertools/blob/master/CHANGELOG.md#0120)
[Release Notes for 0.12.1](https://github.com/rust-itertools/itertools/blob/master/CHANGELOG.md#0121)

### Breaking
- Made `take_while_inclusive` consume iterator by value ([#709](rust-itertools/itertools#709)) --- there are [2 usages](https://www.internalfb.com/code/search?q=repo%3Afbcode%20take_while_inclusive&lang_filter=rust) in fbcode, verified both manually
- Added `Clone` bound to `Unique` ([#777](rust-itertools/itertools#777)) --- there are [37 usages](https://fburl.com/code/hp7vdlch) in fbcode, CI will tell if it breaks

Reviewed By: anps77

Differential Revision: D64305791

fbshipit-source-id: fe99131b206905133c4d2b75693090f5ce44f4ca
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jswrenn jswrenn jswrenn approved these changes

Assignees

No one assigned

Labels

breaking-change

Projects

None yet

Milestone

0.12.0

Development

Successfully merging this pull request may close these issues.

3 participants

@gilhooleyd @Philippe-Cholet @jswrenn