Removed closures in favour of private static methods to close issue #906

[JacobSilasBentley]

This PR:

• primarily closes Avoid closures due to implementations in local functions #906 where closures are to be removed in favour of using private static methods.
• and incidentally fixes Rank does not iterate source on re-iterations #1065

One unit test had to be updated. This was to change the max enumerations of the test sequence from 1 to 2. The test enumerates the sequence twice so this should have always been the case. However I think previously using closures meant that test was enumerated once when it should have been twice. Happy to be corrected if I have misunderstood.

This is my first PR for MoreLINQ so apologies if I have missed any guidance on submitting PRs and will happily take onboard feedback

[codecov]

Codecov Report

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

Project coverage is 93.62%. Comparing base (9e8073d) to head (ad154a8).

Files	Patch %	Lines
MoreLinq/Cartesian.g.cs	28.57%	5 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1064      +/-   ##
==========================================
+ Coverage   92.09%   93.62%   +1.52%     
==========================================
  Files         112      112              
  Lines        3367     3374       +7     
  Branches     1051      957      -94     
==========================================
+ Hits         3101     3159      +58     
  Misses        199      199              
+ Partials       67       16      -51     

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

[atifaziz]

This is my first PR for MoreLINQ so apologies if I have missed any guidance on submitting PRs and will happily take onboard feedback

First and foremost, welcome and thank you for your initiative to help with #906.

One unit test had to be updated. This was to change the max enumerations of the test sequence from 1 to 2. The test enumerates the sequence twice so this should have always been the case. However I think previously using closures meant that test was enumerated once when it should have been twice. Happy to be corrected if I have misunderstood.

Actually you've uncovered a bug that I've documented in #1065! It seems that the sequence was accidentally getting cached by an update of the captured closure. Now that that's not happening with this PR, the test failed. I've proposed the correct fix to the test, which is to materialise the result of Rank before asserting. I've also edited the description of this PR to say it fixes #1065.

While you seem to have addressed the bulk of methods, the following may have slipped:

• Cartesian
• Subsets
• Await
• Batch (Experimental)
• Memoize
• Merge

If we can address those too, as well as some of the feedback, then we should be in a good position to get this merged.

[JacobSilasBentley]

@atifaziz Thank you for the warm welcome and the feedback!

I have hopefully addressed the requested changes and updated the methods that I missed in the initial PR.

'Memoize' might take a little bit more attention as unlike the other methods this method is a member of a non-static class. I'm not entirely sure if there is an advantage to making the method static in this scenario. At least one advantage could just be consistency. I would be interested in your opinion. For now, I have made the method static.

Let me know if there are any further issues for me to address.

[atifaziz]

I have hopefully addressed the requested changes and updated the methods that I missed in the initial PR.

Thank you for all the follow-up!

'Memoize' might take a little bit more attention as unlike the other methods this method is a member of a non-static class. I'm not entirely sure if there is an advantage to making the method static in this scenario. At least one advantage could just be consistency. I would be interested in your opinion. For now, I have made the method static.

Thanks for bringing this case to my attention. I think the right call here may be to simply remove the local function and inline its body. I believe it was separated for clarity rather than consistency and one doesn't need a local function here since GetEnumerator has no arguments and therefore no eager validation is needed.

Let me know if there are any further issues for me to address.

I addressed some minor formatting changes directly (with commits aca7772 and b6608c4) as they were awkward to point out in the PR review UI.

If we can address Memoize then this is good to merge so nearly there! 🏁

[viceroypenguin]

'Memoize' might take a little bit more attention as unlike the other methods this method is a member of a non-static class. I'm not entirely sure if there is an advantage to making the method static in this scenario. At least one advantage could just be consistency. I would be interested in your opinion. For now, I have made the method static.

When re-implementing this class/iterator, I found that splitting the initialization and enumeration code useful. With two methods, neither of which is static, the whole thing became easier to reason about. There are also some corner cases that are not currently covered, such as a reset and re-initialization happening between two MoveNext() calls. You can see my implementation here and feel free to adapt whatever you feel is best.

[JacobSilasBentley]

Thanks for bringing this case to my attention. I think the right call here may be to simply remove the local function and inline its body. I believe it was separated for clarity rather than consistency and one doesn't need a local function here since GetEnumerator has no arguments and therefore no eager validation is needed.

@atifaziz I've changed the method so it is no longer static and following @viceroypenguin suggestion, I've split the initialisation logic and enumeration logic into two different private non-static methods. This has the benefit of the method not being too long. Happy to inline both methods if you would prefer that approach.

@viceroypenguin also mentions:

There are also some corner cases that are not currently covered, such as a reset and re-initialization happening between two MoveNext() calls

I'm happy to look into this but I think it would be more appropriate to do this on a separate issue and separate PR

[atifaziz]

Happy to inline both methods if you would prefer that approach.

@JacobSilasBentley Actually, I would completely revert the change (sorry for misleading) and leave Memoize alone in this PR. The length of the method isn't a real concern and any complexity is incidental (the split even adds more ceremony). I don't think the closure can be avoided here and exists even with the split. The bigger problem with inlining entirely is that it changes the semantics and behaviour and we don't have a test covering that otherwise it would've failed (would be good to eventually add the test). I feel this part of Memoize may need more thought, whether initialisation ought to be lazy as it usually is with most iterator methods, but I'd say #906 doesn't apply in its case. It's not where we're looking to save a closure.

I'm happy to look into this but I think it would be more appropriate to do this on a separate issue and separate PR

Agree, it should be a separate PR, but it would be good to first file those corner cases as demonstratable issues that need prioritising.

[JacobSilasBentley]

@atifaziz I have reverted all changes made to the Memoize method made in this PR.

Let me know if there are any remaining changes required before this code is merged in.

[atifaziz]

LGTM and thank you for all your help with contributing this!