cpp-netlib-devel

[cpp-netlib-devel] URI

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

Hi everyone,

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.

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.
- 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
- A lot more tests need to be added
- 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
- Documentation needs to be updated, and an the example should be
expanded to show the use of scheme() for example
- Another API change is coming up, which will expose the optional
parameters as boost::optionals, instead of returning an empty default
value

Jeroen Habraken

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

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

Re: [cpp-netlib-devel] URI

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

On Tue, Jan 19, 2010 at 11:29 PM, Jeroen Habraken <vex...@gm...> wrote:
>
> On Tue, Jan 19, 2010 at 15:51, Dean Michael Berris
> <mik...@gm...> wrote:
>>
>> 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.
>

Cool. :)

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

That would be great. :)

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

Oh Boost.Test is sooooo easy compared to xUnit testing that it's even
a joke I even invested the time to learn the xUnit tests. Anyway I'm
looking forward to the tests. :)

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

For logging purposes, it's more efficient to just get the stuff after
the ":". I guess it would be better if I log the raw string instead.

Since we're moving to the newer RFC then that should be alright. ;)

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

Yes indeed. I've been meaning to do this but I haven't gotten around
to doing it. :)

>> Thanks for putting in the work and I hope this helps!
>>
>
> I know this is a rather large overhaul which influences the whole
> library, please bear with me and thank you for the response.
>

No worries, as long as it's not broken then it's something that's
worth the investment, it's definitely welcome. :)

-- 
Dean Michael Berris
cplusplus-soup.com | twitter.com/deanberris
linkedin.com/in/mikhailberis | facebook.com/dean.berris | deanberris.com

Re: [cpp-netlib-devel] URI

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

On Tue, Jan 19, 2010 at 16:38, Dean Michael Berris
<mik...@gm...> wrote:
> On Tue, Jan 19, 2010 at 11:29 PM, Jeroen Habraken <vex...@gm...> wrote:
>>
>> On Tue, Jan 19, 2010 at 15:51, Dean Michael Berris
>> <mik...@gm...> wrote:
>>>
>>> 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.
>>
>
> Cool. :)
>
>>>> - 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.
>>
>
> That would be great. :)
>
>>>
>>> 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).
>>
>
> Oh Boost.Test is sooooo easy compared to xUnit testing that it's even
> a joke I even invested the time to learn the xUnit tests. Anyway I'm
> looking forward to the tests. :)
>
>>>
>>> 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?
>>
>
> For logging purposes, it's more efficient to just get the stuff after
> the ":". I guess it would be better if I log the raw string instead.
>
> Since we're moving to the newer RFC then that should be alright. ;)
>
>>> 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.
>>
>
> Yes indeed. I've been meaning to do this but I haven't gotten around
> to doing it. :)
>
>>> Thanks for putting in the work and I hope this helps!
>>>
>>
>> I know this is a rather large overhaul which influences the whole
>> library, please bear with me and thank you for the response.
>>
>
> No worries, as long as it's not broken then it's something that's
> worth the investment, it's definitely welcome. :)
>
> --
> Dean Michael Berris
> cplusplus-soup.com | twitter.com/deanberris
> linkedin.com/in/mikhailberis | facebook.com/dean.berris | deanberris.com
>

FYI, committed HTTP support.

All the initial tests now run, and only the one testing whether
"http://www-.boost.org/" is invalid fails. It turns out the hostname
is valid according to the URI specification, but it's considered
invalid by DNS. Thus when using another name lookup provider such as
the hosts file -/etc/hosts on linux-, it is valid, but I'll have to
double check this.

Jeroen

Re: [cpp-netlib-devel] URI

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

On Wed, Jan 20, 2010 at 4:35 AM, Jeroen Habraken <vex...@gm...> wrote:
>
> FYI, committed HTTP support.
>

Cool!

> All the initial tests now run, and only the one testing whether
> "http://www-.boost.org/" is invalid fails. It turns out the hostname
> is valid according to the URI specification, but it's considered
> invalid by DNS. Thus when using another name lookup provider such as
> the hosts file -/etc/hosts on linux-, it is valid, but I'll have to
> double check this.
>

Oh, if it's valid according to the URI specification then we should be
fine. We can change the docs and the tests to say that we're accepting
these types of URI's but that the DNS system may not be happy with
"malformed" FQDN's.

Thanks Jeroen! Hopefully I'll get a pull request to merge soon.

-- 
Dean Michael Berris
cplusplus-soup.com | twitter.com/deanberris
linkedin.com/in/mikhailberis | facebook.com/dean.berris | deanberris.com