Closed sschuberth closed 5 years ago
/cc @jdillon
@sschuberth I've seen this, I am open to considering a change to include throws IOException, though ATM I'm not sure why its useful/helpful to nest the exceptions Gson throws as IOException, unless you are planning to do something else with the Marshaller?
The current caller of this just passes any Exception up, since there isn't anything else to do here except abort when unmarshal fails for any reason.
I'm not sure why its useful/helpful to nest the exceptions Gson throws as IOException
For once, because the caller then only needs to catch IOExceptions
, and no marshaller-specific exceptions, for error handling.
unless you are planning to do something else with the Marshaller
I do! My plan is to contribute a Jackson-marshaller based on this. With that second marshaller, it makes even more sense to consistently wrap marshaller-specific exceptions into IOExceptions
.
My plan is to contribute a Jackson-marshaller based on this.
FYI, the code is basically ready, and I'll file a PR once this PR is sorted out.
@sschuberth you can open the other PR if you like with these changes and I'll have a look when I have some time.
Have already looked a bit and the Gson case is a bit weird as it already wraps IOException with its JsonIOException, so re-wrapping again is just going to produce a deeper stack-trace.
Ok then @jdillon, if that's already clear I'll simply close this PR unmerged and slightly refactor my Jackson implementation accordingly before opening a new PR. Agreed?
@sschuberth sure, will have to adjust things slightly anyways to get it into a release anyways, so a single PR with both changes is fine. Its on my radar and i'll see what I can do.
I think that adding throws IOException
on Marshaller methods is a no-brainer, and probably should have been like that from the start, but the Gson bits i'm still thinking about, as the Gson folks did that on purpose and re-wrapping will just add a bit of bloat, and decoding will loose a bit of location precision.
And still at the end of the day, any exception thrown IOException or RuntimeException will result is a similar failure handling, though I did find one place that may not have behaved in the DirectoryCache that was expecting only IOException, so its worth looking it over a bit closer wrt exception handling to verify its actually doing the right thing ;-)
Thanks for bringing this to my attention.
Please have a look at the individual commit messages for the details.