Re: My Boost.URL review

From: Vinnie Falco (vinnie.falco_at_[hidden])
Date: 2022-08-22 03:12:40

On Sun, Aug 21, 2022 at 7:58 PM Seth via Boost <boost_at_[hidden]> wrote:
> ...

Thank you for taking the time to review the library.

> In contrast to others, I think the choice of Nomenclature, namely URL
> everywhere _except_ in the grammar domain (the parse function names) is a very
> elegant one: it avoids the historical naming swamp (by choosing the URL camp)
> while still making it abundantly clear what parts of RFC are referenced.

Wow thanks!

> Other libraries don't always expose accurate terms (`authority`, `URI
> reference`) and it leads to confusion, e.g. python3:
>
>     In [1]: from urllib.parse import urlparse
>     In [2]: urlparse('localhost:5555')
>     Out[2]: ParseResult(scheme='', netloc='', path='localhost:5555', params='', query='', fragment='')

That doesn't look quite right, "localhost" should be the scheme and
the path should be "5555" which of course is also not what the user
likely intended but.. yeah.

> I found `resolve` a bit mystifying. While I appreciate the docs not trying to
> duplicate the RFC specifying what the operation means, I think it will be a big
> help to have inspirational examples, like example one that I came up with a
> while ago:

Yes we need more examples.

> It would be welcome to _also_ have `resolve()` member functions for discoverability.

Well, I don't know that I would add a member function that performs an
identical operation just as an alternative... BUT, there is a good
use-case for a member function and that is to allow resolve where the
base is also the destination. This is not currently allowed in the
free function, because the base is received as a view. But as a member
function of the owning containers (url, static_url) it can be done
in-place - one user has already requested this.

> Regarding the parser interface, I feel this should mature into its own library.
> I don't see myself using it currently, and would consider it a pure
> implementation w.r.t. of Boost URL. I have not reviewed this section of the
> library and only skimmed the docs.

A shame because I put the most effort into those docs :) I feel they
are representative of the quality I would like to see for the rest of
the documentation.

> The documentation can use a little work in the narrative department. On the
> plus side, some pages already incorporate some "real life" examples. The
> brevity makes it easy to overlook. Perhaps the examples would be better when
> fleshed out in long form, then linked to.

So just to be clear for everyone participating in the review or who
has plans to use the library, the docs are not yet up to the level I
consider good enough for a Bosot library. We will focus on continuing
to improve the docs.

>  - The current implementation trips a fair number of deprecation warnings in
>    Boost Filesystem - I trust those are probably known and easily addressed

The implementation does not use filesystem but the examples and tests
do, and we have already made a change to silence those warnings but it
is not in master.

>  - In Containers docs: "URL reference. Missing scheme and host." - should say
>    "scheme and authority"
>  - "Although URL query strings are often used to represent key/value pairs,
>    they are not a compound element" - Is confusing to me. It seems to say
>    key/values are not supported, but it goes on to say `url_view::params`
>    extracts "this view of key/value pairs".

Yep, doc work.. never done. You already have visibility into our
workflow so if you want to weigh in while we are polishing that up
feel free :)

>  - Defining BOOST_URL_NO_SOURCE_LOCATION does not compile the tests (possibly more?)

We need to make one of our CI targets set that macro.

>  - message for `error::success` returns arbitrary value:
>     #4372 error.cpp(53) failed: 'BOOST_URL_ERR(error::success).message() == "success"' ('bad alpha' == 'success')

That's weird, can you please open an issue?

>  - "Exceptions only thrown on excessive input length" - what is excessive? Is
>    that restriction useful? Since parsing doesn't allocate in the first place,
>    at the time of parsing any "excessive" allocation will already have existed.
>    Any limit would usefully apply on conversion to `urls::

Yeah I'm not sure what this is about, except for static_url which has
a hard limit on construction.

Thanks