EntilZha / PyFunctional

Python library for creating data pipelines with chain functional programming
http://pyfunctional.pedro.ai
MIT License
2.41k stars 132 forks source link

Fix for the grouped() and to_csv() functions #123

Closed abybaddi009 closed 6 years ago

abybaddi009 commented 6 years ago

Closes #120 and #121 by making the following changes:

I have ran the tests on my Windows machine at work and all are passing. Will check on my linux after reaching home.

EntilZha commented 6 years ago

Added couple comments for adding unit tests. It would be helpful to have these to make sure they original issues were fixed, and prevent future breakage.

I'm also not sure about making the newline argument default to newline, how does the behavior of pyfunctional differ from vanilla python on windows? It might make sense to toss in a tiny bit of platform detection code rather than a default argument for all platforms

abybaddi009 commented 6 years ago

Check the note for newline here. The official documentation says that the default newline argument should be '', so I have changed it from '\n'.

EntilZha commented 6 years ago

Thanks for the csv docs link, that seems to clear things up so I agree that it should be newline='' in to_csv while leaving the universal open function defaulting to None.

I'll update the branch then once tests complete merge. Thanks for the work!

abybaddi009 commented 6 years ago

Thank you so much for letting me work on this. This was my first PR on github.

EntilZha commented 6 years ago

Looks like some tests are failing, but I think the fix is pretty simple. https://travis-ci.org/EntilZha/PyFunctional/jobs/323498016

Basically newline should be None when binary mode is used as per some of the python docs for open. The option is ignored in later versions of python, but throws an error in the backports package. This itself is triggered by the compression related tests.

The simplest fix is probably to use something like if 'b' in mode and if so then set newline to None. Assuming this fixes the tests on travis then I can merge.

EntilZha commented 6 years ago

Almost good to go, could you fix the line to long listing error so that the build succeeds?

abybaddi009 commented 6 years ago

Sure. I will test on my windows machine and then commit in a moment.

codecov-io commented 6 years ago

Codecov Report

Merging #123 into master will decrease coverage by 0.32%. The diff coverage is 61.11%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #123      +/-   ##
==========================================
- Coverage   99.95%   99.63%   -0.33%     
==========================================
  Files          12       12              
  Lines        2168     2183      +15     
==========================================
+ Hits         2167     2175       +8     
- Misses          1        8       +7
Impacted Files Coverage Δ
functional/transformations.py 100% <100%> (ø) :arrow_up:
functional/test/test_functional.py 100% <100%> (ø) :arrow_up:
functional/pipeline.py 100% <100%> (ø) :arrow_up:
functional/test/test_streams.py 98.2% <22.22%> (-1.8%) :arrow_down:

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 7660306...54f6205. Read the comment docs.

EntilZha commented 6 years ago

Looks like the build succeeds now so going to merge. Thanks again for the great work!