Closer to the RFCs defining an URI. Added URI Fragment support.
This now breaks URIs with spaces (which do not follow the RFCs), but these are now required in ign fuel. We must add support for URI encoding before these changes can be merged.
All tests pass on my Windows machine (except [av] and [graphics] components, which I don’t build on Windows).
Tests on my Linux machine including [av] and [graphics] went well.
Thanks for the PR, @peci1. Do you have a suggestion on how to handle encoding? On ign-fuel-tools we use CURL's curl_easy_escape, but it would be nice to have a solution on ign-common which doesn't require CURL.
I think curl_easy_escape would not help if you’d like to escape scheme, path, query and fragment separately… So far, I’ve looked into Mozilla’s source code, and the encoding shouldn’t be that difficult to write. I’ll have a look at it.
A question is how to add it to to exising ign-common code. My idea is adding an optional parameter to URI.Parse() (and URIPath.Parse() and so on) that would tell whether the given string should be encoded or not.
Awesome, thanks for looking into the Mozilla approach. I think that adding a parameter is a good idea, but it can be tricky to keep it API/ABI compatible, so we'd have to go through a deprecation cycle... Maybe a less intrusive option could be to add something like an URI.Encoding()flag which allows setting the encoding type, or maybe just a static URI.Encode() function could also get the job done.
@peci1 , what do you think of removing the whitespace changes from this PR so we can get the fragments in quickly, and revisit the space handling later with a migration plan for ign-fuel-tools? Do the following changes work for you?
I had to decide on what to do in URIPath::PushFront and URIPath::PushBack when the parameter contains slashes. I came to the following solution:
I added explicit methods for setting the URIPath absolute/relative. The methods aren’t virtual, so ABI should not be affected.
if _part is empty: ignore the call at all
if _part starts with a slash
URIPath::PushFront: make the URI absolute, strip the slash, and issue a warning
URIPath::PushBack: if the URI is empty, then make it absolute, strip the slash and issue a warning
if _part has slashes inside (even on the end): the slashes are encoded to %2F
I hope this is a suitable solution. Another option would be to encode even the leading slash and do not treat it special, but I somehow feel this way it might be a bit more user-friendly (e.g. constructs like URIPath path = ”/my” / ”absolute” / ”uri”;).
Thanks for addressing the comments, looking good to me!
I have a suggestion to add a few more checks on this PR. I think we're ready to merge after that.
Approved and fixed a few cppcheck warnings I had missed / caused earlier.