Fix building URIs that have only a scheme and an absolute path #113
Conversation
|
@glynos Would be nice to get this upstream. Not sure if you forgot about it :) |
| ASSERT_EQ("http://example.com/foo", builder.uri()); | ||
| } | ||
|
|
||
| TEST(builder_test, scheme_and_absolute_path) { |
There was a problem hiding this comment.
The test looks OK (and more tests are always good), but the other code modifications in this PR don't affect it. Can you explain what the fix is?
There was a problem hiding this comment.
uri::initialize is called when building URIs from a ur_builder. So the code change is related to this test.
| ++it; | ||
| } | ||
| if (*it == '/' && *(it + 1) == '/') { | ||
| it += 2; |
There was a problem hiding this comment.
Maybe it's me, but I can't see what the difference is here. Is it just an optimisation?
There was a problem hiding this comment.
The code is supposed to skip :// but would also skip :/ like in my example. The resulting URI would have the first slash of the path cut off. You can revert the code and see the result in the unit test.
|
@csk-ableton I thought I added some comments to the review, but I think I got confused by the new interface and they didn't show up. Let me know if you can't see the comments now. |
|
@glynos I saw them and tried to answer your questions. |
glynos
left a comment
glynos
left a comment
There was a problem hiding this comment.
I get it now. To clarify what you fixed, can you add the following lines to this test before I merge it?
ASSERT_EQ("foo", builder.uri().scheme());
ASSERT_EQ("/bar", builder.uri().path());
|
I added the additional assertions and rebased in d6d0f9b |
|
LGTM |
|
Can you make a new release with this fix please? |
3 participants
No description provided.