Reduce time complexity of URL subtraction #1388
Conversation
CodSpeed Performance ReportMerging #1388 will improve performances by ×5.7Comparing Summary
Benchmarks breakdown
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1388 +/- ##
==========================================
+ Coverage 96.05% 96.10% +0.04%
==========================================
Files 31 31
Lines 5784 5822 +38
Branches 361 372 +11
==========================================
+ Hits 5556 5595 +39
+ Misses 202 201 -1
Partials 26 26
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
bdraco
force-pushed
the
url_lazy_no_pathlib
branch
2 times, most recently
from
afea6e5
to
9cae1ef
Compare
bdraco
force-pushed
the
url_lazy_no_pathlib
branch
from
e49c3b6
to
dcac142
Compare
|
@commonism Would you please add some xfail tests for this case? |
|
c.f. #1389 (comment) |
|
@commonism Would you please check if it looks right now? |
| @@ -110,7 +113,7 @@ def test_sub(target: str, base: str, expected: str): | |||
| ( | |||
| "http://example.com////path/////to", | |||
| "http://example.com/////spam", | |||
| "..//path/////to", | |||
| "../path/////to", | |||
There was a problem hiding this comment.
Using ("/path/to", "/spam/", "../path/to") as reference, this can't be right?
There was a problem hiding this comment.
TBH at this point I'm really not sure how its supposed to work and I'm tempted to revert out the whole feature as its seems like its turning out to be more trouble than its worth
There was a problem hiding this comment.
Maybe it is right .. I'm not really sure
http://example.com////path/////to
http://example.com/////spam
There was a problem hiding this comment.
or all the tests are wrong....
There was a problem hiding this comment.
I'd expect it to follow along with the first test case:
…
("/path/to", "/spam/", "../path/to"),
("//path/to", "/spam/", "..//path/to"),
(
"http://example.com//path/to",
"http://example.com/spam",
"..//path/to",
),
(
"http://example.com////path/////to",
"http://example.com/////spam",
"..//path/////to",
)There was a problem hiding this comment.
The trailing / in "spam/" has to be removed.
("/path/to", "/spam", "../path/to"),
("/path/to", "/spam/", "../../path/to"),
And … others to consider
("/path/to", "//spam", "../../path/to"),
(
"http://example.com/path/to",
"http://example.com//spam",
"../../path/to",
),
(
"http://example.com/////path/////to",
"http://example.com////spam",
"../../path/////to",
)
| path = path[:-1] | ||
| if "." in path: | ||
| # Strip '.' segments | ||
| parts = [x for x in path.split("/") if x != "."] |
There was a problem hiding this comment.
stripping "." segments is wrong for "/./" which is equal to "//"
There was a problem hiding this comment.
Maybe right here … but I remember there was a special case on this.
|
I'm think I'm throwing in the towel on this one and going for a revert. We need to start over |
|
@commonism Thanks for catching the issues before we shipped this. I opened #1391 to revert so we can start over with a new approach. |
Time complexity was roughly
O(base_path_segments * target_path_segments)Time complexity is now roughly
O(base_path_segments + target_path_segments)