Skip to content
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

Utility to convert JsResult to Try #41

Merged
merged 2 commits into from Feb 27, 2017

Conversation

cchantep
Copy link
Member

def foo[T](res: JsResult[T]): Try[T] = JsResult.toTry(res)
def bar[T](res: JsResult[T]): Future[T] = Future.fromTry(JsResult.toTry(res))

import play.api.libs.functional._

case class Exception(cause: JsError)
extends IllegalArgumentException(Json stringify JsError.toJson(cause))
Copy link
Member

Choose a reason for hiding this comment

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

Why IllegalArgumentException? RuntimeException would probably be fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

Illegal argument makes more sense, since raised when input data cannot be validated.

Copy link
Member

Choose a reason for hiding this comment

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

I'm just not clear on which method is being passed an invalid argument. See https://docs.oracle.com/javase/8/docs/api/java/lang/IllegalArgumentException.html:

Thrown to indicate that a method has been passed an illegal or inappropriate argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

.validate is passed an invalid JSON argument in this case

Copy link
Member

Choose a reason for hiding this comment

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

Normally you'd use IllegalArgumentException to indicate that input arguments are malformed in some way. It generally indicates a programmer error. I've never seen it used when validating user input and I don't think a typical Scala user would expect that either.

Copy link
Member

Choose a reason for hiding this comment

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

Also, it might be nice to add a toJson method on the exception to make it easy to convert to a result:

lazy val toJson: JsValue = JsError.toJson(cause)

exception.toJson is a lot clearer than JsError.toJson(exception.cause).

Copy link
Member Author

@cchantep cchantep Feb 26, 2017

Choose a reason for hiding this comment

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

"typical Scala user would expect that either." That's not the point, as the Scala dev will sees it's a JsError.Exception, which is quite explicit for me.

The .toJson is not useful, as the Exception will be in case of conversion from JSON validation to Try. The .toJson is only useful when considering the error as validation error.

*
* @tparam T the type for the parsing
* @param result the JSON validation result
* @param err the function to be applied if the results is an error
Copy link
Member

Choose a reason for hiding this comment

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

s/results/result/

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

import play.api.libs.functional._

case class Exception(cause: JsError)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe this should be in JsError? i.e. JsError.Exception is an exception produced from a JsError.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated

@gmethvin gmethvin merged commit 479b9ee into playframework:master Feb 27, 2017
@cchantep
Copy link
Member Author

thx

@cchantep cchantep deleted the feature/41_jsresult_try branch February 27, 2017 09:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants