Fix Issue #53 #57
Fix Issue #53 #57
Conversation
| @@ -96,8 +96,15 @@ public void write(ChannelHandlerContext ctx, Object msg, ChannelPromise promise) | |||
| if (rxRequest.getHeaders().hasContent()) { | |||
| MultipleFutureListener allWritesListener = new MultipleFutureListener(promise); | |||
| allWritesListener.listen(ctx.write(rxRequest.getNettyRequest())); | |||
| if (rxRequest.hasRawContentSource()) { | |||
| RawContentSource<?> rawContentSource = rxRequest.getRawContentSource(); | |||
| ContentSource<?> contentSource = null; | |||
There was a problem hiding this comment.
null initialization is redundant.
|
Apologies, I had to merge my pull request, so you would have to merge. |
Merge branch 'master' of github.com:Netflix/RxNetty Conflicts: src/main/java/io/reactivex/netty/protocol/http/client/HttpRequest.java
| @@ -79,17 +106,46 @@ | |||
| } | |||
|
|
|||
| public HttpRequest<T> withContentSource(ContentSource<T> contentSource) { | |||
| this.contentSource = contentSource; | |||
| if (!contentSet.compareAndSet(false, true)) { | |||
There was a problem hiding this comment.
Since, we are overwriting the content factory, why do we need this check?
The con is that it limits overriding the content later, in case it is to be reused.
There was a problem hiding this comment.
My intention is that only one method from withContent*() can called. User should create new Request object to set a different content source/factory.
There was a problem hiding this comment.
Sure but that constraint is not adding value to the code in terms of state
management, code clarity, etc. OTOH, it is making it difficult to reuse.
So, it seems to me that it is not required. Specially, since this is a
public API, I would recommend removing such a constraint.
On Friday, February 28, 2014, allenxwang [email protected] wrote:
In src/main/java/io/reactivex/netty/protocol/http/client/HttpRequest.java:
@@ -79,17 +106,46 @@
}public HttpRequest<T> withContentSource(ContentSource<T> contentSource) {
this.contentSource = contentSource;if (!contentSet.compareAndSet(false, true)) {My intention is that only one method from withContent*() can called. User
should create new Request object to set a different content source/factory.Reply to this email directly or view it on GitHubhttps://github.com//pull/57/files#r10188652
.
There was a problem hiding this comment.
Fair point. I have removed such checking in pull request
I will add more questions/comments on the issue itself.
Fix Issue #53.
Created new JUnit test for POST with content.