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 AnyUrl.build
doesn't do percent encoding (#3061)
#4224
Conversation
please review
please review |
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 good in principle, can we get some benchmarks to see how much this slows down validation
please update. |
I'm not sure how this affects validation or how to benchmark pydantic's validation, can you assist me on this? |
Thanks so much for the benchmarks, I think we should go with
I think we can skip this, we have to get this right, so there's no option of not quote encoding. The real question long terms is whether we should move this logic to rust, but provided the API doesn't change we could do that in a minor release after V2. |
Okay, I removed
I think this should be moved to rust since I will drop some benchmarks later for Edit: Here's the benchmark, code: This is a 12%ish increase |
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.
Looking good, my main concerns are:
- this could in theory be considered a breaking change, I think it's fine because it's pretty clear than without this the built URLs are "wrong", @PrettyWood do you agree?
- there's no way to set
quote_plus
on a URL field, other thanstricturl
- maybe that's fine?
please can you add to the docs:
- An example of this in action
- an example with
stricturl
of usingquote_plus=False
(or whatever the non-default is) - a "note" admonition saying "percent encoding was added in V1.10 ..."
Please update.
pydantic/networks.py
Outdated
@@ -153,6 +154,7 @@ def __init__( | |||
path: Optional[str] = None, | |||
query: Optional[str] = None, | |||
fragment: Optional[str] = None, | |||
plus: bool = False, |
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.
plus: bool = False, | |
quote_plus: bool = False, |
might be a clearer name.
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.
Also, is there a good reason not to have quote_plus = True
as the default?
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.
Please add quote_plus
to stricturl
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.
Also, is there a good reason not to have
quote_plus = True
as the default?
Just conforming to the standard (RFC 3986)
Also, @FaresAhmedb please remember to use the magic comment "please review" (with one space) to request a review, otherwise we might miss updates on the PR. |
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
please review |
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.
Looking good, just a few small things here, but you need to fix linting in the docs so build succeeds and we can check the docs look right.
Please update.
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
Co-authored-by: Samuel Colvin <samcolvin@gmail.com>
please review |
Thanks so much. |
This reverts commit e34ff92.
Change Summary
percent_encode
function topydantic.utils
percent_encode
AnyUrl.build
AnyUrl.build
Related issue number
fix #3061
Checklist
changes/<pull request or issue id>-<github username>.md
file added describing change