Support owned iterators for GroupBy and IntoChunks into_iter()

[Ten0]

Resolves #499

[jswrenn]

Thanks for the PR! I'm going to defer on reviewing it for a bit, since it appears to be a breaking change, and I want to focus on non-breaking PRs for the near-future.

[erichCompSci]

Hi, I see that this build failure has to do with how the Rust config is set up on the Travis CL continuous integration...but when I pull the offending branch and run locally, I can run the tests and there isn't a problem.

I want this functionality, but actually I want to increase it's capability to an Arc instead of an Rc because I want to use the chunking across an await in an asynchronous function. Still it would probably be good to get this implementation into the code first. Can you advise on how I can get this error on my local machine so I can fix it and send it back to you? Looking at your build statements, it looks like it the std::rc isn't being included in your cargo config possibly? However, when I pulled master mainline into this branch on my local distribution, I still couldn't get this compile error. So, how do I recreate your setup locally, what do I need to do?

[erichCompSci]

Okay, sorry, I delved a little bit more and then I found what commands were being run. I'll work on fixing it

[erichCompSci]

Okay, I fixed it and I can confirm that my one line change made this work and is not a breaking change. However, I am not the original committer. I'm not sure how we could proceed, can you please advise? I would like to get this into mainline if it's not a problem and then I would like to change this to ultimately make IntoChunks Sync and Send.

[Ten0]

Please send the fix and I'll look at it and most likely incorporate it in there. 🙂
However with regards to the Send/Sync change, how do you mean to deal with the Refcell? (which was already there iirc - hard to read quickly on my phone due to this crate not being rustfmt'ed.)

[erichCompSci]

Well, obviously Refcell has to go. You can't be Sync and have RefCell or Cell underneath the hood. Honestly, I need to play around with it a bit some more. Holding references across asyncs call is obviously a horrible idea and I'm still quite new to Pinning. However, I can use iterators across asyncs, so I'm confident a solution exists. I doubt it's a simple as moving to an Arc_Mutex (Couldn't write the < Mutex > for some reason) combination, but we will see. More than that, I know people will be sweating performance implications, so maybe it'd deserve it's own structs and function calls? Just general thoughts, should probably move this conversation somewhere more appropriate.

[Philippe-Cholet]

@Ten0 I only skimmed the changes but I like the idea of this PR (implement IntoIterator for both X and &X), I think it would help solve #665 too.
It would be a (reasonable enough) breaking change though.
I think borrowed_iter should be renamed iter.

[Ten0]

I think borrowed_iter should be renamed iter.

Not generally opposed to it. Just so that we have all elements in mind when making that decision though: I think the reason why I named it this way is because .iter() would suggest that you'd obtain references to the items, and here it's not the case because there's much more complex stuff going on, with interior mutability... so I thought that it was maybe better for clarity to have a naming that signifies that it's an iterator that is borrowed from the chunks but that it's doing something different than yielding references to a precomputed "groups" structure that every other .iter() method in the world does.

With that in mind, do you confirm you want this method to be renamed .iter() ?

[Philippe-Cholet]

I sure see your points, it's well explained. AFAIK, we do not have a reference for this kind of name as the stdlib do not do this kind of stuff. So I don't know. I think you can leave it as borrowed_iter for now.

[codecov]

Codecov Report

Attention: Patch coverage is 83.33333% with 3 lines in your changes are missing coverage. Please review.

Project coverage is 94.44%. Comparing base (6814180) to head (9b3e953).
Report is 99 commits behind head on master.

Files	Patch %	Lines
src/groupbylazy.rs	83.33%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #500      +/-   ##
==========================================
+ Coverage   94.38%   94.44%   +0.06%     
==========================================
  Files          48       49       +1     
  Lines        6665     7062     +397     
==========================================
+ Hits         6291     6670     +379     
- Misses        374      392      +18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Ten0]

@Philippe-Cholet Rebased this.

Not sure what codecov complains about: the lines it says are not covered is a function called by a covered line:
https://app.codecov.io/gh/rust-itertools/itertools/pull/500/blob/src/groupbylazy.rs#L496
Maybe it gets lost because of the inlining, although that shouldn't be different now from before.

[Philippe-Cholet]

Code coverage is an indicator for us and does not make CI fail. Plus, it seems fine on that front.