Re: [boost] boost.url review

wt., 16 sie 2022 o 20:31 Vinnie Falco via Boost napisał(a):

On Tue, Aug 16, 2022 at 11:03 AM Ruben Perez 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.h... 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