-
Notifications
You must be signed in to change notification settings - Fork 1
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix duplicate slashes in url #505
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found. Additional details and impacted files@@ Coverage Diff @@
## main #505 +/- ##
==========================================
+ Coverage 87.96% 88.03% +0.06%
==========================================
Files 24 24
Lines 1030 1036 +6
==========================================
+ Hits 906 912 +6
Misses 124 124
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…shes-in-url' into b/_/zelda-1060/fix-duplicate-slashes-in-url # Conflicts: # pyproject.toml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks awesome. super clear, well tested. Thanks so much for doing this @anthonyburdi
def test_construct_url_from_base_plus_path(base: str, path: str, expected_url: str): | ||
"""Test that urljoin constructs all combinations of urls correctly.""" | ||
constructed_url = urljoin(base, path) | ||
assert constructed_url == expected_url |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to test urljoin
itself
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah you're right - I wrote this test when I had a custom fn. Will clean it up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are getting errors due to duplicate slashes in urls e.g. this error.
This PR is intended to make the agent more resilient when constructing urls. In the future we can also consider using a url object instead of strings, and then construct the string when needed.