Add {Required,Optional}RequestBody readers. #81
Conversation
| object RequiredBody { | ||
|
|
||
| /** | ||
| * Creates a ''RequestReader'' that reads the required required body |
There was a problem hiding this comment.
"... required required body ..."
|
This is pretty cool Ben @BenWhitehead! Could you please all add a couple of unit tests to keep the coverage rate at a good level? |
|
Thanks for the review, I'll incorporate your feedback and add some tests this afternoon. |
| /** | ||
| * A required request body that is interpreted as an ''Array[Byte]'' | ||
| */ | ||
| object RequiredBody { |
There was a problem hiding this comment.
BTW, can't we just do object RequiredBody extends RequestReader ... to use it like:
val r = for {
body <- RequiredBody
}instead of:
val r = for {
body <- RequiredBody()
}and the same for OptionalBody, RequiredStringBody and OptionalStringBody.
| */ | ||
| def apply() = for { | ||
| body <- RequiredBody() | ||
| } yield { new String(body, "UTF-8") } |
There was a problem hiding this comment.
Also, I'm not sure we need these parenthesis here. You can do yield new String(...).
* Update readers to directly extend RequestReader for easier use. * Update readers to minimize code duplication * Cleanup a few stylistic issues
|
Feedback incorporated and I've added unit tests. |
| object RequiredBody extends RequestReader[Array[Byte]] { | ||
|
|
||
| /** | ||
| * Creates a ''RequestReader'' that reads the required body |
There was a problem hiding this comment.
Can we move this paragraph up to object RequiredBody? It's no longer true for apply method.
|
Ben @BenWhitehead, the tests look cool, thanks a lot! Sorry for being picky regarding the code style I'm just trying to keep it as uniform and similar as possible. I'm also realised that we need new exception here: |
Update comment for body readers. Update contributor name. Clean up some minor nitpick items.
|
@vkostyukov I made the requested updates. No worries about being picky, I tend to expect it when contributing to a new code base. |
| it should "produce an error if the body is empty" in { | ||
| val request: HttpRequest = requestWithBody("") | ||
| val futureResult: Future[String] = RequiredStringBody(request) | ||
| intercept[ValidationFailed] { |
There was a problem hiding this comment.
How did it pass if we are raising a BodyNotFound exception?
|
It's actually failed: https://travis-ci.org/finagle/finch/builds/41930575. Don't know why Travis reported success. |
|
Doh, knew I forgot something. Fixed the expected exceptions. All tests pass now. |
|
Thanks Ben @BenWhitehead! This is awesome! |
Add {Required,Optional}RequestBody readers.
No description provided.