Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Make stream seek error message informative #927
Conversation
ronlevine
self-assigned this
Jul 3, 2017
ronlevine
requested a review
from yfarjoun
Jul 3, 2017
|
@yfarjoun Please review. |
codecov-io
commented
Jul 3, 2017
•
Codecov Report
@@ Coverage Diff @@
## master #927 +/- ##
===============================================
+ Coverage 65.118% 65.132% +0.014%
- Complexity 7278 7281 +3
===============================================
Files 528 528
Lines 31971 31975 +4
Branches 5468 5469 +1
===============================================
+ Hits 20819 20826 +7
+ Misses 8995 8994 -1
+ Partials 2157 2155 -2
|
|
I don't understand the claim that this is a more informative error message. this exception is thrown when mFile == null which could mean many things...it could be that the stream has been closed, or it could be that the input stream is non-seekable... |
|
@yfarjoun, do you think that the solution in the following branch is better?: https://github.com/magicDGS/htsjdk/tree/dgs_fix_819 If so, feel free to use it, @ronlevine! |
|
Thanks @magicDGS. That makes the error more specific. |
| @@ -57,10 +57,12 @@ | ||
| public final static String INCORRECT_HEADER_SIZE_MSG = "Incorrect header size for file: "; | ||
| public final static String UNEXPECTED_BLOCK_LENGTH_MSG = "Unexpected compressed block length: "; | ||
| public final static String PREMATURE_END_MSG = "Premature end of file: "; | ||
| - public final static String CANNOT_SEEK_STREAM_MSG = "Cannot seek on stream based file "; |
ronlevine
Jul 4, 2017
Contributor
It's a clearer name but you're right, it's breaks backward compatibility and is not really need. I will revert the name.
ronlevine
Jul 5, 2017
Contributor
I did, it's on the line below. This is the new one. I can reverse the order to make the diff easier to view.
| public final static String INVALID_FILE_PTR_MSG = "Invalid file pointer: "; | ||
| private InputStream mStream = null; | ||
| + private boolean mIsClosed = false; |
magicDGS
Jul 4, 2017
Contributor
Are there other cases where mIsClosed could be used? If so, the CANNOT_SEEK_CLOSED_STREAM_MSG could be re-named to CLOSED_STREAM_MSG and use the String "Closed stream error: ". Then, inside the seek() method, the message could add the "cannot seek a position".
ronlevine
Jul 4, 2017
•
Contributor
Good idea. The generalized message can be used in readLine and read. But, for testing purposes, it should be the base message.
ronlevine
Jul 4, 2017
Contributor
On second thought, I cannot find an application for generalized version of this error message. The logic of readLine and read handle the closed condition appropriately. I added tests to demonstrate this is the case.
| */ | ||
| public void seek(final long pos) throws IOException { | ||
| + // Must be before the mFile == null check because mFile == null for closed files and streams | ||
| + if (mIsClosed) { | ||
| + throw new IOException(CANNOT_SEEK_CLOSED_STREAM_MSG); |
|
@magicDGS Responded to your comments. Please take a look. |
|
Looks good to me. Maybe @yfarjoun have more comments... |
|
@yfarjoun Any more comments? |
| @@ -57,10 +57,12 @@ | ||
| public final static String INCORRECT_HEADER_SIZE_MSG = "Incorrect header size for file: "; | ||
| public final static String UNEXPECTED_BLOCK_LENGTH_MSG = "Unexpected compressed block length: "; | ||
| public final static String PREMATURE_END_MSG = "Premature end of file: "; | ||
| - public final static String CANNOT_SEEK_STREAM_MSG = "Cannot seek on stream based file "; |
ronlevine commentedJul 3, 2017
•
edited
Description
Fixes #819.
Checklist