On Sun, Aug 21, 2022 at 7:58 PM Seth via Boost
...
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