-
Notifications
You must be signed in to change notification settings - Fork 36
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
Parsing JSON body in advance. Solves #391 #392
Conversation
cc @mchimirev |
Example code generated:
|
Ok this introduces a small breaking change since now the body value is not wrapped in F. I might need for backward compatibility to wrap in F with pure, but I will also make the decoded response body available unwrapped (maybe with the name "responseBody"), since it's way nicer to handle (see #391) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, few comments inline.
@@ -184,7 +184,7 @@ private lazy val defaultAsyncHttpClient = PooledHttp1Client() | |||
""") | |||
override val canSerializeUuid = true | |||
override val implicitArgs: Option[String] = None | |||
override val asyncSuccess: String = "now" | |||
override def asyncSuccess: String = "now" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it need to be a def?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It gets overridden
@@ -107,7 +107,13 @@ class ScalaClientMethodGenerator ( | |||
if (response.isUnit) { | |||
s"case r => ${http4sConfig.wrappedAsyncType("Sync").getOrElse(http4sConfig.asyncType)}.${http4sConfig.asyncFailure}(new errors.${response.errorClassName}(r.${config.responseStatusMethod}))" | |||
} else { | |||
s"case r => ${http4sConfig.wrappedAsyncType("Sync").getOrElse(http4sConfig.asyncType)}.${http4sConfig.asyncFailure}(new errors.${response.errorClassName}(r))" | |||
config match { | |||
case _@(_: ScalaClientMethodConfigs.Http4s017 | _: ScalaClientMethodConfigs.Http4s015) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks it can be simplified to
case _: ScalaClientMethodConfigs.Http4s017 | _: ScalaClientMethodConfigs.Http4s015 =>
@@ -145,7 +151,13 @@ class ScalaClientMethodGenerator ( | |||
Some(s"case r if r.${config.responseStatusMethod} == $statusCode => ${http4sConfig.wrappedAsyncType("Sync").getOrElse(http4sConfig.asyncType)}.${http4sConfig.asyncFailure}(new errors.${response.errorClassName}(r.${config.responseStatusMethod}))") | |||
|
|||
} else { | |||
Some(s"case r if r.${config.responseStatusMethod} == $statusCode => ${http4sConfig.wrappedAsyncType("Sync").getOrElse(http4sConfig.asyncType)}.${http4sConfig.asyncFailure}(new errors.${response.errorClassName}(r))") | |||
config match { | |||
case _@(_: ScalaClientMethodConfigs.Http4s017 | _: ScalaClientMethodConfigs.Http4s015) => |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same
} | ||
}.distinct.sorted | ||
|
||
private def exceptionClass( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is only called from one place (line 190) so could be inlined there.
@fiadliel @gheine this is ready now @mchimirev I slightly changed the new API, to avoid doing attempt now you have to use the |
…#392) * Moved error models to client class and removed effect * Added errors object within the client to avoid namespace conflicts * Formatting * Fixed error scope in client * Parsing json body before returning. Solves apicollective#391 * Lifted body into F for backward compatibility and renamed decoded body to 'body' * Inlined exceptionclass and simplified pattern match
No description provided.