-
Notifications
You must be signed in to change notification settings - Fork 72
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
Support reading and writing to/from file-like objects #148
Conversation
try: | ||
from pathlib import Path | ||
except ImportError: | ||
from pathlib2 import Path # python 2 backport |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose for python2 this makes pathlib
or pathlib2
a dependency? Can we just not support python2 after a certain version of cyvcf2? It would make things so much nicer, especially if we can use the typing library for python 3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we need to keep python2 support.
cdef htsFile *hts | ||
cdef bytes fname | ||
cdef bytes mode | ||
cdef bint from_path |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there's probably more in VCF
that's needed across both VCF
and Writer
that we can move here.
@@ -2155,6 +2214,7 @@ cdef from_bytes(s): | |||
return s | |||
|
|||
|
|||
# TODO: make Writer extend HTSFile not VCF by moving common methods into HTSFile |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
^this should be a tech-debt issue
Also: - "/dev/stdin" works as an input to the VCF (reader) class - writing supported from a file descriptor
f88d440
to
7bc4b36
Compare
I kind of wish the |
|
||
# file-like object | ||
with open(VCF_PATH) as fp: | ||
run_writer(Writer(fp, v), f, rec) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm all ears for having a regression test for stdin/stdout support in the reader/writer, but I don't have a good solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks great! thanks Nils.
Is there a flush method on htsFile? might be nice to have that.
@brentp fixed up based on your comments, can you resolve my comments? I am particularly interested in python2 support, and if we can get rid of it! |
@brentp I also noticed that tests are running on travis for this PR, any ideas why not? |
they are running. not sure why they aren't linked: https://travis-ci.org/github/brentp/cyvcf2/jobs/674073923 |
@brentp I had PR in another repo where travis wasn't posting its results. Very odd. |
Build is passing: https://travis-ci.org/github/brentp/cyvcf2/builds/674079601 |
thank you! |
Also: