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

Add Clone trait to Unique #777

Merged
merged 1 commit into from Oct 8, 2023

Conversation

gilhooleyd
Copy link
Contributor

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
Member

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
Member

Philippe-Cholet commented Oct 6, 2023

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

@jswrenn
Copy link
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
Contributor Author

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
Member

jswrenn commented Oct 6, 2023

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

@jswrenn
Copy link
Member

jswrenn commented Oct 6, 2023

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
Contributor Author

Done! Apologies for the churn

@Philippe-Cholet
Copy link
Member

Philippe-Cholet commented Oct 7, 2023

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

EDIT: Done, thanks!

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
8 checks passed
@jswrenn jswrenn mentioned this pull request Nov 14, 2023
@jswrenn jswrenn added this to the next milestone Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants