You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@shindig.apache.org by jo...@gmail.com on 2010/04/07 20:03:14 UTC
Re: Set encoding type for HttpResponseBuilder when setting from
string (issue816045)
one very quick q.
http://codereview.appspot.com/816045/diff/1/3
File
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java
(right):
http://codereview.appspot.com/816045/diff/1/3#newcode79
java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:79:
public HttpResponseBuilder setEncoding(Charset charset) {
looks like we intend to make this an implementation detail.
package-private for testing purposes?
http://codereview.appspot.com/816045/show
Re: Set encoding type for HttpResponseBuilder when setting from
string (issue816045)
Posted by John Hjelmstad <fa...@google.com>.
LGTM
Seems reasonable. Patching.
On Wed, Apr 7, 2010 at 11:11 AM, Ziv Horesh <zh...@gmail.com> wrote:
> On Wed, Apr 7, 2010 at 11:03 AM, <jo...@gmail.com> wrote:
>
> > one very quick q.
> >
> >
> > http://codereview.appspot.com/816045/diff/1/3
> > File
> >
> >
> >
> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java
> > (right):
> >
> > http://codereview.appspot.com/816045/diff/1/3#newcode79
> >
> >
> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:79:
> > public HttpResponseBuilder setEncoding(Charset charset) {
> > looks like we intend to make this an implementation detail.
> > package-private for testing purposes?
>
> Since this is a builder, I can see cases that someone might want to use it,
> so I don't see a reason why to hide it.
> I don't see it as implementation detail, but more of a service.
>
>
> >
> >
> > http://codereview.appspot.com/816045/show
> >
>
Re: Set encoding type for HttpResponseBuilder when setting from
string (issue816045)
Posted by Ziv Horesh <zh...@gmail.com>.
On Wed, Apr 7, 2010 at 11:03 AM, <jo...@gmail.com> wrote:
> one very quick q.
>
>
> http://codereview.appspot.com/816045/diff/1/3
> File
>
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java
> (right):
>
> http://codereview.appspot.com/816045/diff/1/3#newcode79
>
> java/gadgets/src/main/java/org/apache/shindig/gadgets/http/HttpResponseBuilder.java:79:
> public HttpResponseBuilder setEncoding(Charset charset) {
> looks like we intend to make this an implementation detail.
> package-private for testing purposes?
Since this is a builder, I can see cases that someone might want to use it,
so I don't see a reason why to hide it.
I don't see it as implementation detail, but more of a service.
>
>
> http://codereview.appspot.com/816045/show
>