Refactor URL parsing code #7464
Conversation
|
@kkaefer, thanks for your PR! By analyzing this pull request, we identified @jfirebaugh, @tmcw and @yhahn to be potential reviewers. |
kkaefer
force-pushed
the
7464-refactor-url-parsing
branch
2 times, most recently
from
9f28972
to
b5d528f
Compare
| if (extensionIdx == std::string::npos || extensionIdx == queryIdx - 1) { | ||
| return url; | ||
| std::string | ||
| canonicalizeTileURL(const std::string& str, const SourceType type, const uint16_t tileSize) { |
There was a problem hiding this comment.
The prior version of canonicalizeTileURL wasn't restricted to *.mapbox.com, right? It definitely needs to continue to support *.mapbox.cn (we should add a test for this) and should likely continue to support arbitrary/staging hostnames as well. It's only used in situations where the source URL is mapbox://..., so I don't think enforcing a particular hostname is necessary.
|
👍 on |
| EXPECT_EQ(Path::Segment({ 18, 0 }), URLPath("http://example.com?query=foo.bar").directory); | ||
| } | ||
|
|
||
| TEST(Path, Extension) { |
There was a problem hiding this comment.
Should have a test for double extensions (e.g. .vector.pbf).
There was a problem hiding this comment.
I guess it's debatable on what constitutes an extension and what is part of the filename. For a (hypothetical) URL of mapbox://sources/mapbox.mapbox-terrain-v2,mapbox.mapbox-streets-v7, would the extension be nothing? or .mapbox-terrain-v2,mapbox.mapbox-streets-v7?
There was a problem hiding this comment.
Looks like you went with "the final period and everything following". Seems good to me!
kkaefer
force-pushed
the
7464-refactor-url-parsing
branch
from
b5d528f
to
5a544b6
Compare
|
For reference, here's an insightful article by the author of cURL on why RFC compliance for URL parsing doesn't work: https://daniel.haxx.se/blog/2016/05/11/my-url-isnt-your-url/ |
An attempt at #6183.