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

Addressing Issue 158: Unexpected Sequence Wrapping from Grouped #160

Merged
merged 1 commit into from Jul 5, 2021
Merged

Addressing Issue 158: Unexpected Sequence Wrapping from Grouped #160

merged 1 commit into from Jul 5, 2021

Conversation

stephan-rayner
Copy link
Contributor

Elements That are Lists in a Sequence Will Remain Lists When Grouped.

Much of the details around this PR can be found in issue 158. I added a quick summary below to get everyone on the same page.

from functional import seq
s = [[i,i+1] for i in range(10)]
for batch in seq(s).grouped(3):
    for item in batch:
        print(f'{item} -- {type(item)}')

Running the above code sample and printing the output (were it captured) will result in the following output.

[0, 1] -- <class 'functional.pipeline.Sequence'>
[1, 2] -- <class 'list'>
[2, 3] -- <class 'list'>
[3, 4] -- <class 'functional.pipeline.Sequence'>
[4, 5] -- <class 'list'>
[5, 6] -- <class 'list'>
[6, 7] -- <class 'functional.pipeline.Sequence'>
[7, 8] -- <class 'list'>
[8, 9] -- <class 'list'>
[9, 10] -- <class 'functional.pipeline.Sequence'>

The same check run with these changes produces the output below.

[0, 1] -- <class 'list'>
[1, 2] -- <class 'list'>
[2, 3] -- <class 'list'>
[3, 4] -- <class 'list'>
[4, 5] -- <class 'list'>
[5, 6] -- <class 'list'>
[6, 7] -- <class 'list'>
[7, 8] -- <class 'list'>
[8, 9] -- <class 'list'>
[9, 10] -- <class 'list'>

Special thanks to @evanw2 for his leg work on this issue. The discovery in the first place and all of the investigation made this a lot easier #ShouldersOfGiants.

Elements that are lists in a sequence will remain lists when grouped.
Additionally, cleaning up as per the suggestions of black.
@codecov
Copy link

codecov bot commented Jul 4, 2021

Codecov Report

Merging #160 (e81d110) into master (6c50426) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #160   +/-   ##
=======================================
  Coverage   97.97%   97.98%           
=======================================
  Files          12       12           
  Lines        2226     2237   +11     
=======================================
+ Hits         2181     2192   +11     
  Misses         45       45           
Impacted Files Coverage Δ
functional/pipeline.py 98.08% <100.00%> (ø)
functional/test/test_functional.py 98.93% <100.00%> (+0.01%) ⬆️
functional/transformations.py 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c50426...e81d110. Read the comment docs.

@evanw2
Copy link

evanw2 commented Jul 4, 2021

Thanks for making this! Looks good to me.

@EntilZha
Copy link
Owner

EntilZha commented Jul 5, 2021

LGTM as well, thanks for the fix!

@EntilZha EntilZha merged commit 56ff25b into EntilZha:master Jul 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants