Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Added the ability to query the length of the line terminator to Ascii… #843
Conversation
| @@ -93,6 +105,10 @@ public final String readLine(final PositionalBufferedStream stream) throws IOExc | ||
| if (c == LINEFEED || c == CARRIAGE_RETURN) { | ||
| if (c == CARRIAGE_RETURN && stream.peek() == LINEFEED) { | ||
| stream.read(); // <= skip the trailing \n in case of \r\n termination | ||
| + this.lineTerminatorLength = 2; | ||
| + } | ||
| + else { |
tfenne
Apr 5, 2017
Owner
How meta ;) In all seriousness though I thought putting the else onto the next line was user-choice in htsjdk & picard. Certainly the rule used to be the opposite (always put it on a new line).
yfarjoun
Apr 5, 2017
Contributor
I don't know about "used to be" I was trained by @nh13 to always have } else { I'm not super picky, but I thought it appropriate due to the subject matter.
tfenne
Apr 5, 2017
Owner
Since I've been around since the inception of htsjdk, I'm pretty sure I know about "used to be"
codecov-io
commented
Apr 5, 2017
•
Codecov Report
@@ Coverage Diff @@
## master #843 +/- ##
===============================================
+ Coverage 64.916% 64.931% +0.015%
- Complexity 7200 7204 +4
===============================================
Files 527 527
Lines 31784 31789 +5
Branches 5425 5425
===============================================
+ Hits 20633 20641 +8
+ Misses 9016 9015 -1
+ Partials 2135 2133 -2
|
| + | ||
| + @Test public void voidTestLineEndingLength() throws Exception { | ||
| + final String input = "Hello\nThis\rIs A Silly Test\r\nSo There"; | ||
| + final InputStream is = new ByteArrayInputStream(input.getBytes()); |
yfarjoun
Apr 5, 2017
Contributor
What should happen if there's a stray carriage return with no linefeed? Could you add that to the test?
yfarjoun
Apr 5, 2017
Contributor
also, I don't know what happens with the wrong order \n\r but it should be tested...
tfenne
Apr 5, 2017
Owner
Seriously? Valid line endings in use today are CR, LF, CR+LF. AsciiLineReader specifically looks for those three endings for lines. The test already covers all three valid line ending (a solo CR, a solo LF and a CR+LF).
If they are in the wrong order, they will be treated as multiple lines (existing behaviour). I.e. foo\n\r would be tokenized as <foo><eol><empty><eol>.
yfarjoun
Apr 5, 2017
Contributor
I'm not asking to modify the valid line-endings...I just want the tests to be explicit about it.
So the test will show an empty line and the correct size of line ending.
| @@ -49,4 +41,20 @@ public void testReadLines() throws Exception { | ||
| assertEquals(expectedNumber, actualLines); | ||
| } | ||
| + | ||
| + @Test public void voidTestLineEndingLength() throws Exception { | ||
| + final String input = "Hello\nThis\rIs A Silly Test\r\nSo There"; |
magicDGS
Apr 5, 2017
Contributor
I think that will be nice to have also the ' \r\n' at the end for testing.
|
Great @tfenne! This is perfect for the .fai index. Thank you! |
|
Could this be merged for using it for the fai creator? Thanks in advance! :) |
|
|
|
needs a rebase..sorry. |
yfarjoun
was assigned
by droazen
Apr 18, 2017
|
@yfarjoun Rebased and squashed. Ok to merge once the tests pass? |
|
|
tfenne commentedApr 5, 2017
…LineReader.
Description
I wanted @magicDGS to have access to the line terminator length so that😉
.faicreation could be more robustChecklist