Re: [cpp-netlib-devel] URI

[Dean M. B. <mik...@gm...>]

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.

> - 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.

> - 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.

> - 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.

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.

> - 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.

> - 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.

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