Closed wasade closed 4 years ago
Both StringIO
and BytesIO
are standard Python mechanisms for handling IO streams. I'm not sure how Qiita uses this package, but it would be a good idea for files to be opened in binary as to not use str
. That was true before these changes though. The py27 logic for bytes / str is confusing.
Finally going back to this; just to clarify, my concern is that due to the way tests are written and the fundamental differences between Python2-3, it might be possible that when we actually read a file from disk the code will not behave as expected (ran into some of this when converting Qiita). Anyway, to test this I downloaded this PR code and:
copied one of the tests
echo '@a_1 orig_bc=abc new_bc=abc bc_diffs=0
xyz
ABC @b_1 orig_bc=abw new_bc=wbc bc_diffs=4 qwe
DFG @b_2 orig_bc=abw new_bc=wbc bc_diffs=4 qwe
DEF ' > fastq.fastq
ran it in the terminal
>>> from qiita_files.util import open_file
>>> from qiita_files.demux import to_hdf5, to_ascii_file
>>> from h5py import File
>>>
>>> with File('fastq.demux', 'w') as f:
... to_hdf5(['fastq.fastq'], f)
...
>>> to_ascii_file('fastq.demux', 'fastq1.fastq')
checked results:
$ cat *.fastq
@a_1 orig_bc=abc new_bc=abc bc_diffs=0
xyz
ABC @b_1 orig_bc=abw new_bc=wbc bc_diffs=4 qwe
DFG @b_2 orig_bc=abw new_bc=wbc bc_diffs=4 qwe
DEF
@a_0 orig_bc=abc new_bc=abc bc_diffs=0 xyz + ABC @b_0 orig_bc=abw new_bc=wbc bc_diffs=4 qwe + DFG @b_1 orig_bc=abw new_bc=wbc bc_diffs=4 qwe + DEF
Which AFAIK looks correct, right? Do you want someone else to review? Anyway, before merging, it will be good to tag the current version, in case we need it for something else (I'll do that), then figure out which plugins will need to change.
Don't the to_hdf5
and to_ascii_file
tests that already exist cover this? These tests use h5py.File
, open
and make a lot of use real files via tempfile
That's correct and realized that when putting that example together. IMOO this is ready to merge but wondering if you would like someone else to review before? BTW I already created the tag/release so we have a python2.7 in case we need it in the future.
Okay thanks. I don't think another reviewer is necessary as this is general maintenance
Great! Merging now.
Resolves warnings / deprecations, and removes py27 support