search

gbif / gbif-common

Utility classes
Apache License 2.0
1 stars 1 forks source link

TabularDataFileReader.close should throw IOException #3

Closed ansell closed 7 years ago

ansell commented 7 years ago

TabularDataFileReader currently defines its close method without the ability to throw an IOException, but it also overrides java.io.Closeable, which is otherwise allowed to throw an IOException from its close method. 

void close();

It would be simpler if TabularDataFileReader didn't change the close method definition. The reasoning is that close method implementations will either need to silently swallow/log (not ideal), "sneaky rethrow" (not ideal), or wrap exceptions as RuntimeException (won't be handled in the same way as IOException by client code).

The reasoning in the AutoCloseable documentation is that implementors should feel free to drop the checked exception from their throws clause if they know that they will not be thrown. ( https://docs.oracle.com/javase/7/docs/api/java/lang/AutoCloseable.html ) However, TabularDataFileReader's will be dealing with IO in almost all situations, except for in-memory implementations, and hence should be free to simply let their IOException's propagate as expected to somewhere that the final user can handle.

timrobertson100 commented 7 years ago

@ansell - is your work here related to the current effects of line breaks in the AU data? Please be aware we're also exploring a more lenient CSV read in the dwc-io project (@cgendreau leading that)

ansell commented 7 years ago

Yes, I am working through a case where a users DWcA has an extension file with a double-quoted field and a line break in one. It silently succeeded and gave errors further on from here because the fields were transposed after the error because of not failing early

With the Java 9 stuff I was testing this week to see how far I could get before things broke down, ended up getting to the Scala-2.10 dependencies that we use this library in.

On 2 Jun 2017, at 3:38 pm, Tim Robertson notifications@github.com wrote:

@ansell - is your work here related to the current effects of line breaks in the AU data? Please be aware we're also exploring a more lenient CSV read in the dwc-io project (@cgendreau leading that)

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

cgendreau commented 7 years ago

Indeed, TabularDataFileReader should not override Closeable close method. SuperCsvFileReader implementation should also not log the issue on close but let the caller handle it. Both fixed now. Thanks for reporting the issue.