Skip to content

Cleanup URL percent-encoding behavior. #2990

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.

Sign up for GitHub

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

Merged
lovelydinosaur merged 13 commits into from
Dec 15, 2023
+113 −42
Merged

Cleanup URL percent-encoding behavior. #2990

Changes from 1 commit
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
9c669b7
Replace path_query_fragment encoding tests
lovelydinosaur Dec 7, 2023
6cc7555
Remove replaced test cases
lovelydinosaur Dec 7, 2023
b7d4258
Fix test case to use correct hex sequence for 'abc'
lovelydinosaur Dec 7, 2023
412a880
Fix 'quote' behaviour so we don't double-escape.
lovelydinosaur Dec 7, 2023
a1b4afc
Add '/' to safe chars in query strings
lovelydinosaur Dec 7, 2023
de9968a
Update docstring
lovelydinosaur Dec 11, 2023
7bdf227
Linting
lovelydinosaur Dec 11, 2023
76ec6eb
Merge branch 'master' into cleanup-url-encoding
lovelydinosaur Dec 11, 2023
dd7b657
Merge branch 'master' into cleanup-url-encoding
karpetrosyan Dec 13, 2023
0d8cc4b
Update outdated comment.
lovelydinosaur Dec 13, 2023
e3532aa
Revert unrelated change
lovelydinosaur Dec 13, 2023
84f6338
Merge branch 'master' into cleanup-url-encoding
lovelydinosaur Dec 13, 2023
53d15f1
Merge branch 'master' into cleanup-url-encoding
lovelydinosaur Dec 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Update docstring
  • Loading branch information
@lovelydinosaur
lovelydinosaur committed Dec 11, 2023
commit de9968a849c30b51e17b06d6e72e4cfadc2e0453
7 changes: 7 additions & 0 deletions httpx/_urlparse.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -451,6 +451,13 @@ def percent_encoded(string: str, safe: str = "/") -> str:
def quote(string: str, safe: str = "/") -> str:
Copy link
Copy Markdown
Contributor

@karpetrosyan karpetrosyan Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe we should also add unit tests for these functions, rather than simply testing them with httpx.URL.
This would be a more robust approach, in my opinion.

Copy link
Copy Markdown
Contributor

@T-256 T-256 Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO current approach is fine.

This is clearly a code-smell, because our test cases ought to be tests against our public API, rather than testing implementation details. Perhaps there's some cases where it's a necessary hack, but... perhaps not?

from https://github.com/encode/httpx/issues/2492#issue-1478857204

Copy link
Copy Markdown
Contributor

@zanieb zanieb Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree testing the public API should be sufficient unless something private is particularly expensive to test via the public API.

Copy link
Copy Markdown
Contributor

@karpetrosyan karpetrosyan Dec 14, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's a matter of preference, but if we encounter regression in our httpx.URL tests, we will go through these functions and find the method that isn't working properly, which is why unit tests are useful.

"""
Use percent-encoding to quote a string, omitting existing '%xx' escape sequences.

See: https://www.rfc-editor.org/rfc/rfc3986#section-2.1

* `string`: The string to be percent-escaped.
* `safe`: A string containing characters that may be treated as safe, and do not
need to be escaped. Unreserved characters are always treated as safe.
See: https://www.rfc-editor.org/rfc/rfc3986#section-2.3
"""
Comment thread
lovelydinosaur marked this conversation as resolved.
parts = []
current_position = 0
Expand Down