Re: [cpp-netlib-devel] URI

[Jeroen H. <vex...@gm...>]

Hi,

A few quick responses,

On Tue, Jan 19, 2010 at 15:51, Dean Michael Berris
<mik...@gm...> wrote:
> Hi Jeroen,
>
> On Tue, Jan 19, 2010 at 1:08 AM, Jeroen Habraken <vex...@gm...> wrote:
>>
>> I just pushed a major rewrite of the URI parser to my github fork, and
>> there are a couple of things to take into consideration for the
>> moment. Also, Dean, could you please verify I pushed it to the right
>> branch and everything this time.
>>
>
> I see it in your 0.5-devel branch. Hopefully that'd be easier to merge
> into my 0.5-devel branch but I have a few concerns.
>
>> Here's a quick list, in no specific order:
>> - HTTP support is broken at the moment, which breaks a lot of other
>> code, this should be fixed soon and is a priority.
>
> When do you expect this to be available? I'm curious to see how that's
> going to shape up granted that there's no more way to customize the
> parsing of the specific parts.

I've nearly got this working, so either today or tomorrow I hope.

>> - The parser doesn't have support for IP's yet, thus
>> scheme://localhost/ works, scheme://127.0.0.1/ doesn't, whilst both
>> are valid
>
> These were valid before IIRC and there must be tests to show that they
> are supported.

They are indeed valid, but I've not written the parser rules for them.
I shall add a couple of tests reflecting this.

>> - A lot more tests need to be added
>
> I would better prefer that there be tests written first before any
> work is done so that I can look at the tests and see whether the
> feature to be added makes sense. The way I do it is I force test
> failures first before I go write functionality.
>
> Please look up Test Driven Development and see if you can follow that
> process instead. For one thing I will not commit a merge if tests are
> failing.

Yes, and I'm aware of TDD, but unfamiliar with Boost.Test. There are a
couple of tests I've written outside the framework which need to be
merged, for this I'll have to look into Boost.Test though (not that
that's a bad thing).

>> - There's been an API change, protocol() has been renamed to scheme()
>> as it's what the name the RFC uses, and according to Wikipedia scheme
>> and protocol are two different things
>
> Alright, but I see that 'rest()' was removed too. The value of getting
> the raw scheme-specific part of a URI is something I personally
> require and prefer be still there.

The newer RFC (http://www.ietf.org/rfc/rfc3986.txt) which deprecates
RFC1738 has no notion of a scheme-specfic-part, which makes this
rather hard. Could you please explain why you need it?

> This is the main reason why I put that protocol()/scheme() and rest()
> functions in the basic_uri type. The raw() accessor should also be
> there so that a URI's raw string representation can be retrieved even
> after the parts have been parsed already.

I don't believe it was there initially, but it's trivial to add and makes sense.

>> - Documentation needs to be updated, and an the example should be
>> expanded to show the use of scheme() for example
>
> Yes, if you can do that too before I merge your work that would be
> most appreciated too.

Sure.

>> - Another API change is coming up, which will expose the optional
>> parameters as boost::optionals, instead of returning an empty default
>> value
>>
>
> Please update the tests and documentation to show that things have changed.

Of course.

> Thanks for putting in the work and I hope this helps!
>
> --
> Dean Michael Berris
> cplusplus-soup.com | twitter.com/deanberris
> linkedin.com/in/mikhailberis | facebook.com/dean.berris | deanberris.com
>

I know this is a rather large overhaul which influences the whole
library, please bear with me and thank you for the response.

Jeroen