Tolerate empty annotation file, throw better error#612
Conversation
BryonLewis
left a comment
BryonLewis
left a comment
There was a problem hiding this comment.
More a comment about how the determine no file vs bad file
| if (rawBuffer.length < 5) { | ||
| /** | ||
| * 5 is somewhat arbitrary, and accounts for newline chars | ||
| * This cannot possibly be a valid file. Return empty | ||
| */ | ||
| return {}; |
There was a problem hiding this comment.
I'm not exactly sure how I feel about this. A file that is truly empty or a file that is just {} feels different than a file that contains 12345 or null (it shouldn't have it but who knows) or something else that falls within that 5 limit but cannot be parsed. Feels like it should error then. Should we have explicit cases where it works: Missing file, blank file or empty object file?
There was a problem hiding this comment.
I'm guarding against newline characters that make the file look empty.
I think you're right that this is too permissive. Perhaps we should strip newlines from the input before parsing, then safely handle the 0 length case, and otherwise throw an error.
There was a problem hiding this comment.
Overthinking it. Need to handle the empty file case, but otherwise, I think a parser error is appropriate.
BryonLewis
left a comment
BryonLewis
left a comment
There was a problem hiding this comment.
As long as we throw the error telling them why, this looks good.
subdavis
added
skipped-planning
2 participants
don't think we'll hear back on this issue. Either way, this is a problem.