You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@hc.apache.org by Michael Becke <mb...@gmail.com> on 2005/04/16 05:31:29 UTC
Some 4.0 Comments
Hi Oleg,
I've been going over all the new 4.0 code and it looks great! I'm
getting anxious to jump in and start coding. Overall I agree with
everything you've done so far.
Below are some comments/ideas/questions that came to mind as I was
looking over the code. Some of them are a little trivial, but I
wanted to write them down before I forgot.
- I like the change to the HttpParams. We can probably even take it
a little further by making the Http*Params methods static and just do
things like the following:
HttpParams params = new HttpParams();
HttpConnectionParams.setConnectionTimeout(params, 1);
We could then rename either HttpParams or the Http*Params classes to
differentiate them.
- Should getInputStream() be a part of HttpEntity? Adding an
HttpIncomingEntity might be better as the InputStream is only really
necessary for the response and on the server side.
- Header.parse() and HeaderElement.parseElements() should probably be
moved to HeaderParser.
- Why does HttpLineParser.readRawLine() specify that the input stream
must be unchunked?
- Is EntityConsumer just a utility class or does it have a bigger
purpose? If it's just utility we should probably move it to
o.a.h.util.
- Why are the variables of AbstractHttpConnection transient? Do we
anticipate serializing connection?
- To be consistent we should rename HeadersParser to HeaderParser.
- It seems a little overkill, but to be consistent with
HttpMessage/HttpMutableMessage we should probably separate the set/get
methods in HttpEntityEnclosingMessage.
- The third assertEquals() in TestEncodingUtils.testBytesToString()
doesn't work for me as my default charset is ASCII. I would go ahead
and change this one, but I'm not sure what you were trying to test.
I'm going to continue going over the new code and also start filling
in some of the missing spaces.
Nice work Oleg!
Mike
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
Re: Some 4.0 Comments
Posted by Michael Becke <mb...@gmail.com>.
Sounds good. I was playing with entities last night as well to
familiarize myself with the code. I'm looking forward to seeing what
you come up with.
Mike
On 4/16/05, Oleg Kalnichevski <ol...@apache.org> wrote:
> Hi Mike,
>
> Thanks for the feedback. It is really appreciated.
>
> Can we hold off the discussion of individual points fora little while? I
> am really trying hard to finish the HttpServerConnection and HttpEntity
> related classes by the end of the weekend, so we have a feature complete
> implementation to work with. A lot of things will be much clearer once
> the server side of the story is taken into consideration
>
> Oleg
>
> Once we have more or less complete picture, I'll address
>
>
> On Fri, 2005-04-15 at 23:31 -0400, Michael Becke wrote:
> > Hi Oleg,
> >
> > I've been going over all the new 4.0 code and it looks great! I'm
> > getting anxious to jump in and start coding. Overall I agree with
> > everything you've done so far.
> >
> > Below are some comments/ideas/questions that came to mind as I was
> > looking over the code. Some of them are a little trivial, but I
> > wanted to write them down before I forgot.
> >
> > - I like the change to the HttpParams. We can probably even take it
> > a little further by making the Http*Params methods static and just do
> > things like the following:
> >
> > HttpParams params = new HttpParams();
> > HttpConnectionParams.setConnectionTimeout(params, 1);
> >
> > We could then rename either HttpParams or the Http*Params classes to
> > differentiate them.
> >
> > - Should getInputStream() be a part of HttpEntity? Adding an
> > HttpIncomingEntity might be better as the InputStream is only really
> > necessary for the response and on the server side.
> >
> > - Header.parse() and HeaderElement.parseElements() should probably be
> > moved to HeaderParser.
> >
> > - Why does HttpLineParser.readRawLine() specify that the input stream
> > must be unchunked?
> >
> > - Is EntityConsumer just a utility class or does it have a bigger
> > purpose? If it's just utility we should probably move it to
> > o.a.h.util.
> >
> > - Why are the variables of AbstractHttpConnection transient? Do we
> > anticipate serializing connection?
> >
> > - To be consistent we should rename HeadersParser to HeaderParser.
> >
> > - It seems a little overkill, but to be consistent with
> > HttpMessage/HttpMutableMessage we should probably separate the set/get
> > methods in HttpEntityEnclosingMessage.
> >
> > - The third assertEquals() in TestEncodingUtils.testBytesToString()
> > doesn't work for me as my default charset is ASCII. I would go ahead
> > and change this one, but I'm not sure what you were trying to test.
> >
> > I'm going to continue going over the new code and also start filling
> > in some of the missing spaces.
> >
> > Nice work Oleg!
> >
> > Mike
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
> > For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
> >
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
>
>
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
Re: Some 4.0 Comments
Posted by Oleg Kalnichevski <ol...@apache.org>.
Hi Mike,
Thanks for the feedback. It is really appreciated.
Can we hold off the discussion of individual points fora little while? I
am really trying hard to finish the HttpServerConnection and HttpEntity
related classes by the end of the weekend, so we have a feature complete
implementation to work with. A lot of things will be much clearer once
the server side of the story is taken into consideration
Oleg
Once we have more or less complete picture, I'll address
On Fri, 2005-04-15 at 23:31 -0400, Michael Becke wrote:
> Hi Oleg,
>
> I've been going over all the new 4.0 code and it looks great! I'm
> getting anxious to jump in and start coding. Overall I agree with
> everything you've done so far.
>
> Below are some comments/ideas/questions that came to mind as I was
> looking over the code. Some of them are a little trivial, but I
> wanted to write them down before I forgot.
>
> - I like the change to the HttpParams. We can probably even take it
> a little further by making the Http*Params methods static and just do
> things like the following:
>
> HttpParams params = new HttpParams();
> HttpConnectionParams.setConnectionTimeout(params, 1);
>
> We could then rename either HttpParams or the Http*Params classes to
> differentiate them.
>
> - Should getInputStream() be a part of HttpEntity? Adding an
> HttpIncomingEntity might be better as the InputStream is only really
> necessary for the response and on the server side.
>
> - Header.parse() and HeaderElement.parseElements() should probably be
> moved to HeaderParser.
>
> - Why does HttpLineParser.readRawLine() specify that the input stream
> must be unchunked?
>
> - Is EntityConsumer just a utility class or does it have a bigger
> purpose? If it's just utility we should probably move it to
> o.a.h.util.
>
> - Why are the variables of AbstractHttpConnection transient? Do we
> anticipate serializing connection?
>
> - To be consistent we should rename HeadersParser to HeaderParser.
>
> - It seems a little overkill, but to be consistent with
> HttpMessage/HttpMutableMessage we should probably separate the set/get
> methods in HttpEntityEnclosingMessage.
>
> - The third assertEquals() in TestEncodingUtils.testBytesToString()
> doesn't work for me as my default charset is ASCII. I would go ahead
> and change this one, but I'm not sure what you were trying to test.
>
> I'm going to continue going over the new code and also start filling
> in some of the missing spaces.
>
> Nice work Oleg!
>
> Mike
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
Re: Some 4.0 Comments
Posted by Oleg Kalnichevski <ol...@apache.org>.
Hi Mike,
See my comments in-line
On Fri, 2005-04-15 at 23:31 -0400, Michael Becke wrote:
> Hi Oleg,
>
> I've been going over all the new 4.0 code and it looks great! I'm
> getting anxious to jump in and start coding. Overall I agree with
> everything you've done so far.
>
> Below are some comments/ideas/questions that came to mind as I was
> looking over the code. Some of them are a little trivial, but I
> wanted to write them down before I forgot.
>
> - I like the change to the HttpParams. We can probably even take it
> a little further by making the Http*Params methods static and just do
> things like the following:
>
> HttpParams params = new HttpParams();
> HttpConnectionParams.setConnectionTimeout(params, 1);
Non-static methods may have their advantages as well. This is matter of
taste, but some people tend to find this kind of chained method
invocations more readable.
HttpParams params = new DefaultHttpParams(null);
new HttpConnectionParams(params)
.setSocketTimeout()
.setConnectTimeout()
.setParam1()
.setParam2()
.serParam3();
I do not have a strong opinion here and can easily live with any of the
two approaches
>
> We could then rename either HttpParams or the Http*Params classes to
> differentiate them.
>
> - Should getInputStream() be a part of HttpEntity? Adding an
> HttpIncomingEntity might be better as the InputStream is only really
> necessary for the response and on the server side.
>
Agreed. Already implemented. Unfortunately there's a catch. If we want
to keep HttpEntityEnclosingRequest applicable to both client- and
server-side request objects, we'll have to cast
HttpEntityEnclosingRequest#getEntity to HttpIncomingEntity in order to
get access to the input stream, which is quite ugly. Alternatively we
may consider providing HttpOutgoingEntityEnclosingRequest interface for
the client side request objects and HttpIncomingEntityEnclosingRequest
interface for the server side objects plus its mutable counterpart, at
which point things really start getting grotesque. I am still trying to
come up with a better solution, which would eliminate redundant cast
from HttpEntity to HttpIncomingEntity and would not result in zillion of
additional interfaces.
> - Header.parse() and HeaderElement.parseElements() should probably be
> moved to HeaderParser.
>
I do not have a strong opinion here. I personally tend to prefer Header,
HeaderElement, StatusLine and RequestLine each to have a static parse
method of its own, but I can live with all those methods moved to one
UberParser class
> - Why does HttpLineParser.readRawLine() specify that the input stream
> must be unchunked?
No clue. Just copied from Commons HttpClient trunk verbatim. Javadocs is
whole different story. As soon as we are all _more_ or _less_ happy with
the new API we can deal with the javadocs quality.
>
> - Is EntityConsumer just a utility class or does it have a bigger
> purpose? If it's just utility we should probably move it to
> o.a.h.util.
Agreed.
>
> - Why are the variables of AbstractHttpConnection transient? Do we
> anticipate serializing connection?
>
My bad. They were meant to be volatile
> - To be consistent we should rename HeadersParser to HeaderParser.
Fine by me
>
> - It seems a little overkill, but to be consistent with
> HttpMessage/HttpMutableMessage we should probably separate the set/get
> methods in HttpEntityEnclosingMessage.
Agreed
>
> - The third assertEquals() in TestEncodingUtils.testBytesToString()
> doesn't work for me as my default charset is ASCII. I would go ahead
> and change this one, but I'm not sure what you were trying to test.
>
Most likely I was just trying to keep Clover happy.
> I'm going to continue going over the new code and also start filling
> in some of the missing spaces.
That would be fantastic! My intention now is step back a little and let
others to carefully review the new API, correct what is not right, and
contribute the missing bits. Meanwhile I'll be working on providing
better test coverage, examples and javadocs
Cheers,
Oleg
>
> Nice work Oleg!
>
> Mike
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
> For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org
>
---------------------------------------------------------------------
To unsubscribe, e-mail: httpclient-dev-unsubscribe@jakarta.apache.org
For additional commands, e-mail: httpclient-dev-help@jakarta.apache.org