[HOLD] Expose and test "_with_parser" variants of the "consume_seqfile" method#1785
[HOLD] Expose and test "_with_parser" variants of the "consume_seqfile" method#1785standage wants to merge 3 commits into
Conversation
| deref(self._ht_this).consume_seqfile[CpFastxReader](parser.parser, | ||
| total_reads, | ||
| n_consumed) | ||
| return total_reads, n_consumed |
There was a problem hiding this comment.
Cut n pasted further down.
standage
changed the title
|
Ready for review and merge! @ctb @camillescott @luizirber @betatim |
|
Ahhh hrmm. So this is already addressed in #1774... if you don't want to finish up that PR, maybe it would be best to cherry pick some commits, or follow its formatting? It removes the with_parser variants entirely, and instead takes an |
|
I think I only mentioned it in Slack... probably should have made a note in the title comment. |
Great. I would only be too happy to close this PR in favor of that one.
Sure, I can take a look. Generally I have a habit of ignoring most PRs until review is requested unless 1) I'm particularly interested in its progress or 2) I'm mentioned by name. I was under the impression that this was standard operating procedure.
Yeah, I must have missed the Slack message. I might have paid more attention if I had realized you had started down this route in that PR. I'll take a closer look and comment here later. |
Is this actually part of #1774? I only see changes to the tests, not to the relevant method implementations. |
|
Yeah, definitely in there -- probably just didn't get expanded by default for being a big diff (had to click the button when I looked) |
Yep. :-/ This looks a lot better than what I was trying to do. I'll pick up from here. |
|
Sounds good.
Most is done already -- sandbox scripts need to be converted, and there are
two streaming tests that are hanging on Thread joining which I marked skip
for now. Once sandbox is switched over, this PR can remove all the
remaining CPython wrapper.
…On Wed, Sep 13, 2017 at 5:04 PM, Daniel Standage ***@***.***> wrote:
probably just didn't get expanded by default for being a big diff
Yep. :-/
------------------------------
This looks a lot better than what I was trying to do. I'll pick up from
here.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1785 (comment)>, or mute
the thread
<https://github.com/notifications/unsubscribe-auth/ACwxrRaMNqPayisvBLLpbxlNOMugoje4ks5siG17gaJpZM4PW1-4>
.
--
Camille Scott
Graduate Group for Computer Science
Lab for Data Intensive Biology
University of California, Davis
camille.scott.w@gmail.com
|
Codecov Report
@@ Coverage Diff @@
## master #1785 +/- ##
======================================
Coverage 0.06% 0.06%
======================================
Files 78 78
Lines 9792 9792
Branches 2457 2457
======================================
Hits 6 6
Misses 9786 9786
Continue to review full report at Codecov.
|
standage
changed the title
|
Superseded and fixed by #1787. |
3 participants
Threaded processing requires using the parser variant rather than the filename variant. This PR exposes the parser variant of all the "consume_seqfile" methods. In an ideal world, Cython wouldn't have bugs and would be able to handle this cleanly. Until such a time, we've duplicated each consume function at the Cython level.
Related to #1771. Doesn't close, but temporarily addresses the issue so that we can actually use the functionality at the Python level.
Also included in the PR.
_with_reads_parserto_with_parserand compensatory changes to scripts/, sandbox/, tests/, etc.make testDid it pass the tests?make clean diff-coverIf it introduces new functionality inscripts/is it tested?make format diff_pylint_report cppcheck doc pydocstyleIs it wellformatted?
additions are allowed without a major version increment. Changing file
formats also requires a major version number increment.
documented in
CHANGELOG.md? See keepachangelogfor more details.
changes were made?
tested for streaming IO?)