Skip to content
This repository has been archived by the owner on Oct 25, 2024. It is now read-only.

Add custom exception class to fix todo from #152 #154

Closed
wants to merge 2 commits into from

Conversation

nesterov-n
Copy link

This PR contains "todo" fixes from issue #152.
Custom exception type TapeException is introduced to wrap low-level IOException

@@ -557,7 +557,7 @@ private void checkForComodification() {
// Return the read element.
return buffer;
} catch (IOException e) {
throw new RuntimeException("todo: throw a proper error", e);
throw new RuntimeException("Error is occurred while reading object", e);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be a TapeException too?

/** Thrown when an internal error has occurred in tape lib **/
public class TapeException extends RuntimeException {

private static final long serialVersionUID = 5138225684096988531L;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this used for?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throwable implements Serializable.
That being said, I'd prefer not to empower such usage.

f2prateek added a commit that referenced this pull request Jul 28, 2018
This is the same as Java 8's [UncheckedIOException](https://docs.oracle.com/javase/8/docs/api/java/io/UncheckedIOException.html). We've copied it so that we can maintain compatibility with Java 7.

Other options:
* Continue to throw RuntimeException with a nicer error message (i.e. remove the "todo")
* Create a custom exception Tape class like we had in tape v1 (ref #154)

I think this is nicer than the second as it keeps the ergonomics of Java 8 for those stuck on having to support Java 7.
@f2prateek
Copy link
Collaborator

Closed by #193

@f2prateek f2prateek closed this Aug 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants