Streamline file reading to use io streams#498
Open
Ivorforce wants to merge 3 commits intoMIT-LCP:mainfrom
Open
Streamline file reading to use io streams#498Ivorforce wants to merge 3 commits intoMIT-LCP:mainfrom
Ivorforce wants to merge 3 commits intoMIT-LCP:mainfrom
Conversation
… rdann, in case the file is not read from disk.
e0f7da4 to
52054f2
Compare
2 tasks
25c3704 to
e6277a4
Compare
d7fc7f0 to
9de2d92
Compare
…d annotation files. These methods all now stack between the public functions, handling mainly i/o, and the parsing functions.
9de2d92 to
7a83307
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I've recently needed to read files from buffers rather than files (streamed from file selection pickers). I've opened up a Pull Request with a quick fix allowing a buffer override: #491
However, a simple buffer override is suboptimal. Ideally, there are as few diverging paths in the code as possible. Additionally, it makes sense to separate concerns of parsing and I/O, because being agnostic to the buffer source enables flexibility and decreases complexity on both sides. Hence, merging this PR is much preferred to #491.
This Pull Request implements my recommendation for a restructure of
rdrecord,rdheaderandrdann.rdannwas easy to modify, butrdrecordandrdheader's ability to read multi-segment records does not easily translate to reads using streams. This results in code that isn't as nice as I was hoping - so someone needing to read data from buffers must therefore implement surrounding functionality, such as pre-reading the header or acting on multi-segment records, themselves. Still, streamlining the functions addressed in this PR already facilitate accomplishing this goal at all, without needing to re-write all of the parsing functionality of wfdb.Before Merging
This pull request needs:
Concerns and observations
_rdheaderspecifying the encoding.iso-8859-1ascii_rdheaderaccepting a pre-decoded string, since the file is plaintext. I advocate a binary input though to avoid inconsistent coding behavior.rdheaderpreviously ignored file read errors for local files. I would like input on whether that was intentional, because a file read error could result in corrupted file reads.rdheaderpreviously allowed passing a directory even ifpn_dirwas supplied. In this case, thefile_namedirectory was cut from the record name and silently ignored. I don't think this is the right call, so I cut this functionality. Let me know if there's a reason to keep it.bufferingarguments are, as of now, removed. It should be easy to re-introduce them into the new enclosing functions, if needed.no_fileparameter could be deprecated, instead using aNonetest against sig_data. This removes a potential failure case because it's unlikely someone passingsig_datadoesn't intend it to be used.