New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add throws annotation FPromiseHelper#get #1227

Merged
merged 1 commit into from May 13, 2014

Conversation

Projects
None yet
5 participants
@kamekoopa
Contributor

kamekoopa commented Jun 18, 2013

add throws annotation.
Promise#get() throws TimeoutException, but can't know it from java api.
excepion handling is inconvenience.

@cloudbees-pull-request-builder

This comment has been minimized.

cloudbees-pull-request-builder commented Jun 18, 2013

play2-master-PRs #247 SUCCESS
This pull request looks good

@jroper

This comment has been minimized.

Member

jroper commented Jun 25, 2013

Not sure about this. I think I'd rather document that it throws a TimeoutException.

@kamekoopa

This comment has been minimized.

Contributor

kamekoopa commented Jun 26, 2013

try {
  promise.get();
} catch (TimeoutException e){
  //error handling
}

The above is compile error, because Promise#get() method does not have the throws signature and a TimeoutException is a checked exception.
In order to handle an error, have to write like the below and I have felt inconvenient this is.

try {
  promise.get();
} catch(Throwable e){
  if (e instanceof TimeoutException){
    //error handling
  }
}
@richdougherty

This comment has been minimized.

Member

richdougherty commented Jul 4, 2013

I think the best solution is if get() threw an unchecked exception, but that would be an API change. (Adding a throws annotation is an API change too.)

@jroper

This comment has been minimized.

Member

jroper commented Oct 16, 2013

Let's make it throw an unchecked exception, create a new TimeoutException class that extends RuntimeException, and wraps TimeoutException thrown by promise.get().

@jroper

This comment has been minimized.

Member

jroper commented Oct 16, 2013

@kamekoopa are you able to do this?

@kamekoopa

This comment has been minimized.

Contributor

kamekoopa commented Oct 17, 2013

@jroper I feel good your idea.

I made an attempt to make modifications perform the its idea in a framework side.
But if you cannot accept this by reasons of influence size and so on, I intend to solve it in the application side.

@jroper

This comment has been minimized.

Member

jroper commented Apr 30, 2014

@kamekoopa It seems I missed your last comment 7 months ago.

The changes you've made are good, except for one thing, the play.core package is intended for internal APIs. If you move PromiseTimeoutException to the F class, then we should be good to go.

There's also been extensive refactoring of Promise since you made the original changes, so it might be a bit of work to rebase this on the current master. Additionally, once you've done the work, would you be able to squash your commits?

If you're not in a position to be able to do these updates, just let me know.

Wrapped the TimeoutException in the unchecked exception
Wrapped the TimeoutException of `Promise#get()` in the unchecked exception for timeout handling.
@lightbend-cla-validator

This comment has been minimized.

lightbend-cla-validator commented May 12, 2014

Hi @kamekoopa,

Thank you for your contribution! We really value the time you've taken to put this together.

Before we proceed with reviewing this pull request, please sign the Typesafe Contributors License Agreement:

http://www.typesafe.com/contribute/cla

@kamekoopa

This comment has been minimized.

Contributor

kamekoopa commented May 12, 2014

@jroper Thank you for replying!

According to your comment, I performed rebase and squash of commits.

@kamekoopa

This comment has been minimized.

Contributor

kamekoopa commented May 12, 2014

@typesafehub-validator I had signed the CLA.

jroper added a commit that referenced this pull request May 13, 2014

@jroper jroper merged commit 9c12a3e into playframework:master May 13, 2014

1 check passed

default Merged build finished.
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment