Add config setting to avoid adding trailing slash to URLs#1719
Add config setting to avoid adding trailing slash to URLs#1719samuelcolvin wants to merge 4 commits intomainfrom
Conversation
CodSpeed Performance ReportMerging #1719 will not alter performanceComparing Summary
|
| Bound(Bound<'py, PyUrl>), | ||
| Owned(PyUrl), |
There was a problem hiding this comment.
@davidhewitt is there a better way of doing this, or a better name?
There was a problem hiding this comment.
The point of this is to avoid allocating the Python object until it's absolutely needed. Maybe call it LazyUrlObject and have the variants be
| Bound(Bound<'py, PyUrl>), | |
| Owned(PyUrl), | |
| Object(Bound<'py, PyUrl>), | |
| Struct(PyUrl), |
| } | ||
|
|
||
| #[pymethods] | ||
| impl PyUrl { |
There was a problem hiding this comment.
Possibly query #[getter] also needs this treatment?
| Bound(Bound<'py, PyUrl>), | ||
| Owned(PyUrl), |
There was a problem hiding this comment.
The point of this is to avoid allocating the Python object until it's absolutely needed. Maybe call it LazyUrlObject and have the variants be
| Bound(Bound<'py, PyUrl>), | |
| Owned(PyUrl), | |
| Object(Bound<'py, PyUrl>), | |
| Struct(PyUrl), |
| } | ||
| } | ||
|
|
||
| impl CopyFromPyUrl for EitherUrl<'_> { |
There was a problem hiding this comment.
Seeing this again, this looks like it probably should have been AsRef<Url> and AsMut<Url> implementations.
Viicos
left a comment
Viicos
left a comment
There was a problem hiding this comment.
PyUrl.path will still contain the trailing slash:
u = Url('https://example.com', add_trailing_slash=False)
u.path
#> '/'Do we also want to remove it here?
| s | ||
| } | ||
|
|
||
| pub fn __repr__(&self) -> String { |
There was a problem hiding this comment.
Does __repr__() also need to be updated?
| } | ||
| _ => s, | ||
| }; | ||
| if remove_trailing_slash && s.ends_with('/') { |
There was a problem hiding this comment.
I think the point is to preserve the input, not always add or always strip. Can we keep track of the input ending with a trailing slash?
If not the config option is confusing: it should be remove_trailing_slash: bool = False to match the implementation.
There was a problem hiding this comment.
It seems that it is working as expected but I'm not entirely sure why:
u = Url('https://example.com/', add_trailing_slash=False)
str(u)
#> 'https://example.com/'
Pydantic's AnyUrl incorrectly adds trailing slashes to base URLs without paths. This behavior is being fixed in pydantic-core PR #1719 and will be available in Pydantic 2.12+. Instead of working around the issue, mark the test as expected to fail. This documents the known bug and will automatically alert when Pydantic fixes it (the test will XPASS with xfail_strict=true in pyproject.toml). Ref: pydantic/pydantic-core#1719
|
Studying this, I think that it's possibly more complex than it needs to be and that the main problem case is that "empty" path gets replaced with So I think we might instead just need a simpler config option like |
|
Superseded by #1789 |
4 participants
Preparation for fixing pydantic/pydantic#7186