Skip to content

Import BytesIO from io #519

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

Merged
merged 1 commit into from
Mar 22, 2017
Merged

Import BytesIO from io #519

merged 1 commit into from
Mar 22, 2017

Conversation

effigies
Copy link
Member

>>> import nibabel
[...]/nibabel/cifti2/parse_cifti2.py:24: FutureWarning: We no longer carry a copy of the 'six' package in nibabel; Please import the 'six' package directly
  from ..externals.six import BytesIO

This PR also updates a bunch of from six import BytesIO imports I found while making sure that was the only thing still importing from externals.six.

@matthew-brett
Copy link
Member

Looks good - are you happy with just this change, or are there more to come?

Would you mind rebasing?

@effigies
Copy link
Member Author

Yeah, I'll rebase before this goes in. I'm a little curious why that first one failed, so I want to run these tests. I'll ping you once I've stopped tinkering.

@codecov-io
Copy link

codecov-io commented Mar 22, 2017

Codecov Report

Merging #519 into master will not change coverage.
The diff coverage is 100%.

@@           Coverage Diff           @@
##           master     #519   +/-   ##
=======================================
  Coverage   94.14%   94.14%           
=======================================
  Files         175      175           
  Lines       23887    23887           
  Branches     2568     2568           
=======================================
  Hits        22489    22489           
  Misses        918      918           
  Partials      480      480
Impacted Files Coverage Δ
nibabel/tests/test_image_load_save.py 96.33% <100%> (ø) ⬆️
nibabel/tests/test_arraywriters.py 96.2% <100%> (ø) ⬆️
nibabel/tests/test_files_interface.py 97.05% <100%> (ø) ⬆️
nibabel/tests/test_arrayproxy.py 100% <100%> (ø) ⬆️
nibabel/tests/test_fileholders.py 100% <100%> (ø) ⬆️
nibabel/freesurfer/tests/test_mghformat.py 100% <100%> (ø) ⬆️
nibabel/streamlines/tests/test_trk.py 100% <100%> (ø) ⬆️
nibabel/tests/test_wrapstruct.py 92.23% <100%> (+0.02%) ⬆️
nibabel/cifti2/parse_cifti2.py 82.96% <100%> (ø) ⬆️
nibabel/streamlines/tests/test_streamlines.py 95.36% <100%> (ø) ⬆️
... and 10 more

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 6f58e02...c381801. Read the comment docs.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.157% when pulling d0ea42e on effigies:remove_six into 6f58e02 on nipy:master.

@matthew-brett
Copy link
Member

I did try this refactoring a while back, but ran into the same trouble and backed off.

The differences come from the fact thatfrom six import BytesIO, StringIO both result in StringIO.StringIO in Python 2.7. StringIO.StringIO accepts and returns not-unicode strings; as expected by the tests - that causes some of the errors. io.BytesIO can't be pickled, causing another error. I can't remember the explanation for the other errors, but I concluded that it wasn't worth refactoring round them. Maybe you'll come to a different conclusion though...

@effigies
Copy link
Member Author

Yeah, not planning on refactoring, but I do want to see which ones are safe to switch. Latest commit looks to be building fine on Travis. At least now we're down to:

$ grep -rI IO nibabel | grep 'six import'
nibabel/externals/tests/test_six.py:        from six import BytesIO
nibabel/tests/test_analyze.py:from six import BytesIO, StringIO
nibabel/tests/test_batteryrunners.py:from six import StringIO
nibabel/tests/test_wrapstruct.py:from six import StringIO

If someone decides to tackle those last few imports, at least the scope will be clearer.

Squashing...

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.157% when pulling d0bb37d on effigies:remove_six into 6f58e02 on nipy:master.

@effigies
Copy link
Member Author

Rebased. I'll merge when (assuming) the tests pass, unless you'd rather do the honors?

@matthew-brett
Copy link
Member

Go for it.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 96.157% when pulling c381801 on effigies:remove_six into 6f58e02 on nipy:master.

@effigies effigies merged commit ca977ab into nipy:master Mar 22, 2017
@effigies effigies deleted the remove_six branch March 22, 2017 20:45
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.

4 participants