Boost logo

Boost :

From: Vinnie Falco (vinnie.falco_at_[hidden])
Date: 2022-08-22 20:14:37


On Mon, Aug 22, 2022 at 11:56 AM Christopher Bläsius via Boost
<boost_at_[hidden]> wrote:
> I hope this is OK (I won't give a recommendation to accept or reject though).

Yes it is great! Feedback from the trenches is highly valued.

> One of the more common scenarios in our applications is the construction
> of urls...I feel like the current API design lacks a bit in this regard.

Now that I am looking at your example code, yes I agree. I don't like
some things that I'm seeing. We would be very happy to work with you
and fine tune the current API to work better for your use case. To
start, I would instead prefer to see your example written thusly:

> auto url = boost::urls::url{};
> url.set_scheme("https")
> .set_host("special-api.com")
> .set_port("443")
> .set_path("/prefix/api/v1/");
> url.segments().append({ "applications", applicationId, "devices" });
> url.params().append("since", "2022-01-01");

> The url::set_... methods seem to follow a fluent api design which is
> nice, but unfortunately they return url_base&.

Hmm yeah I see what you mean. I didn't think of this - that's why
field experience is so vital. The reason they return url_base& is
because that's the class that has all the mutation logic. This is
shared between `url` and `static_url`. To get functions like
`set_host` to return `url&` instead would be challenging. We would
have to put a copy of the entire set of mutating API member function
signatures into both `url` and `static_url_base` and have those return
the correct type.

> So I can't construct a
> url inside a function call like:
>
> httpClientGet(url{}.set_host(host).set_path("some/path"))

What if there was a helper function or a helper constructor? For
example that takes the five parts and assembles them into one URL:

    // here, scheme, query, and fragment are `none`
    httpClientGet( url( none, host, "some/path", none, none );

> Being able to add segments is great. Personally I'm a big fan of
> overloading the /-operator to append paths like in the filesystem
> library, because I think this results in more readable code. The example
> of above could become:
>
> httpClientGet(url{host} / "some/path")

This is going to have the same problem, operator/ will return
url_base&. I agree the operators are nice. We were thinking to first
get some more field experience and fine tune the current APIs, and
then once things are stable - pour some operators into the mix.

> it is a shame that there are no semantic types to go along with
> them. ...The documentation states that the base url has to satisfy
> the absolute-URI grammar. Wouldn't it be great to have a distinct type
> for this?

That would be a challenge to design and implement. Consider the following:

    absolute_uri u( "http://example.com" );

what happens if I then write:

    u.remove_scheme();

Does this return a new type? If the library offers uri and
relative_ref as separate semantic types, what would
parse_uri_reference return, given that its grammar is:

    URI-reference = URI / relative-ref

Would the return type be `result< variant< uri_view, relative_ref_view
> >`? I explored the use of alternative types when I first started
writing the library but it became obviously fairly quickly that it
places significant constraints on the resulting APIs, and gets quite
unergonomic. Not to mention, that there would be an enormous
duplication of function signatures and the documentation that goes
with it.

While the current design loses the compile-time distinctions between
the various flavors of compliant URLs, it does gain convenience as a
vocabulary type. Each container is capable of holding any valid
string, and users never have to worry about the type changing upon
conversion.

> BTW why is there only an authority_view and not an owning container?
> Seems inconsistent to me.

The authority_view is a concession to users of HTTP, in order to parse
request-target. The authority_view grammar is called "authority-form"
in rfc7230:

     request-target = origin-form
                    / absolute-form
                    / authority-form
                    / asterisk-form

Implementing authority_view was quite simple. We made a copy of
url_view and just trimmed away the stuff that wasn't relevant. But
implementing an authority container with mutation operations, would
not be as simple. We could have done it, and nothing stops us from
adding it in the future, but I do not have sufficient evidence that
such a container would be as popular or widely used as the main url
containers. If enough users clamour for this feature in the future
then we might add it.

Generating a valid URL can be quite an undertaking. But generating a
valid authority is a lot more simple. Especially since we provide the
ipv4 and ipv6 address classes which will correctly format those
address types for you. So while the url containers offer significant
value in assisting users in producing valid URLs, we do not feel that
an "authority" container would offer as much additional value beyond
what the user can already accomplish by composing the authority string
themselves. Furthermore there are far fewer circumstances when a user
has to build an authority, compared to when they have to build a URL.

> Its nice that there are functions to manually do percentage encoding,
> but I would prefer (additional) simpler functions which just return a
> std::string.

Yeah, that's fair. We have on our todo to review and refactor the
percent-encoding suite of functions.

> It would be nice if the library also provided the default port numbers
> for the schemes. I know the documentation states that this is not part
> of the URL-protocol, but I gonna need that information if I use the library.

That's already been added to the develop branch!

> I would recommend not to start your example page with a 700 lines
> implementation of magnet links, that's super intimidating.

LOL! point taken...

> Something I missed was a simple example showing that the parse functions
> also accept unknown schemes like opc.tcp:// for example.

Yes that's a good idea

> Thanks a lot for all your work on providing useful high quality
> libraries for everyone!

Thank you for taking the time to review the library!

Regards


Boost list run by bdawes at acm.org, gregod at cs.rpi.edu, cpdaniel at pacbell.net, john at johnmaddock.co.uk