Re: boost.url review

From: Andrzej Krzemienski (akrzemi1_at_[hidden])
Date: 2022-08-17 18:12:43

wt., 16 sie 2022 o 20:31 Vinnie Falco via Boost <boost_at_[hidden]>
napisał(a):

> On Tue, Aug 16, 2022 at 11:03 AM Ruben Perez <rubenperez038_at_[hidden]>
> wrote:
> > Here is my review of Boost.Url.
>
> Thank you for graciously contributing your time to review the library!
>
> > I've found the naming for view containers confusing (e.g.
> > params and params_view).
>
> Well...yeah. I am not entirely happy about having 8 different
> containers between the segments and params ranges. However the
> decision was made to offer the user an interface that resembles
> standard containers, so that standard algorithms could be composed on
> them (more or less). I think that the ForwardRange and
> BidirectionalRange interfaces were the right choice, so we followed
> the design where it led us. The need for having read-only views,
> modifiable views, over separate encoded versus unencoded strings left
> us with the 8 containers.
>
> > I would either emphasize it in the docs, or rename the containers to
> > something that reflects this fact (e.g. const_params and mutable_params,
> > as asio does).
>
> I would much rather emphasize it in the docs, because writing "const"
> and "mutable" over and over is bound to be a chore. Asio gets away
> with it because the mutability of buffers is central to the role of
> asynchronous operations but I'm not so sure the same applies here.
>
> We are very happy to hear proposals for alternate container names,
> especially if they are complete proposals (give an alternative for
> each existing name) and if necessary what the url_view and url member
> functions would be renamed to.
>
> > I'd consider exposing higher-level functions here, maybe taking a lvalue
> > reference to a std::string, allowing the user to percent-encode strings
> easier
> > (so they resemble JavaScript encodeURIComponent).
>
> Yes, I am not joyous about our current percent-encoding APIs. There's
> a lot of them, with small variations, and some simple operations are
> missing (appending or assigning to a MutableString out-param as you
> pointed out). They are still great but I think they could be greater.
> We will work on them some more, and if you or anyone else would like
> to provide input into that process, it is welcomed.
>
> > I don't know if this is a common use case, but I'd consider adding a
> function
> > that simplifies it (i.e. params::set_value(string_view key, string_view
> value).
>
> Yep, we need that! There are open questions (like what do you do if
> more than one key exists). Here is an open issue for it:
>
> <https://github.com/CPPAlliance/url/issues/412>
>
> > I'm not very keen on exposing the parsing infrastructure and helper
> > functions (other than percent encoding/decoding) as public API,
> > as I think it violates the principle of API surface minimization.
>
> I feel you there but they have to go somewhere because the
> alternatives are worse. It isn't perfect, I know.
>

Maybe an alternative would be to keep them in the public API but give them
a separate namespace, like `urls::encoding`. This would indicate more
clearly that you have a relatively separate sub-library there, that needs
to stay for logistic reasons. Boost.Lexical_cast took a similar approach
for try_lexical_convert:
https://www.boost.org/doc/libs/1_80_0/doc/html/boost_lexical_cast/synopsis.html#boost_lexical_cast.synopsis.try_lexical_convert

Regards,
&rzej;

> > The documentation is good and the reference is complete, but I miss
> > some examples.
>
> Yep. Its not easy to come up with a complete application that just
> does stuff with URLs.
>
> > I'd add a page and an example on percent encoding functions, as they
> > are only explained in the reference and the explanation assumes
> > familiarity with the CharSet concept.
>
> Yes I think these routines need some tidying and docs.
>
> > I'm also curious about the security review that is announced
> > in the main page - when is that going to take place?
>
> Once the code settles down a little, if the library is accepted then
> the version which first appears in Boost would be reviewed. Possibly
> the second release if the company doing the review is very busy.
>
> Regards
>
> _______________________________________________
> Unsubscribe & other changes:
> http://lists.boost.org/mailman/listinfo.cgi/boost
>