- Notifications
You must be signed in to change notification settings - Fork293
Add config setting to avoid adding trailing slash to URLs#1719
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
base:main
Are you sure you want to change the base?
Conversation
codspeed-hqbot commentedMay 24, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
CodSpeed Performance ReportMerging#1719 willnot alter performanceComparing Summary
|
| Bound(Bound<'py,PyUrl>), | ||
| Owned(PyUrl), |
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.
@davidhewitt is there a better way of doing this, or a better 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.
The point of this is to avoid allocating the Python object until it's absolutely needed. Maybe call itLazyUrlObject and have the variants be
| Bound(Bound<'py,PyUrl>), | |
| Owned(PyUrl), | |
| Object(Bound<'py,PyUrl>), | |
| Struct(PyUrl), |
| implFrom<PyUrl>forUrl{ | ||
| fnfrom(value:PyUrl) ->Url{ | ||
| value.lib_url | ||
| } | ||
| } | ||
| #[pymethods] | ||
| implPyUrl{ |
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.
Possiblyquery#[getter] also needs this treatment?
| Bound(Bound<'py,PyUrl>), | ||
| Owned(PyUrl), |
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.
The point of this is to avoid allocating the Python object until it's absolutely needed. Maybe call itLazyUrlObject and have the variants be
| Bound(Bound<'py,PyUrl>), | |
| Owned(PyUrl), | |
| Object(Bound<'py,PyUrl>), | |
| Struct(PyUrl), |
| EitherUrl::Py(py_url) =>Ok(py_url), | ||
| EitherUrl::Rust(rust_url) =>Bound::new(py,PyUrl::new(rust_url)), | ||
| EitherUrl::Bound(py_url) =>Ok(py_url), | ||
| EitherUrl::Owned(py_url) =>Bound::new(py,py_url), | ||
| } | ||
| } | ||
| } | ||
| implCopyFromPyUrlforEitherUrl<'_>{ |
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.
Seeing this again, this looks like it probably should have beenAsRef<Url> andAsMut<Url> implementations.
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.
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?
| ifself.remove_trailing_slash && s.ends_with('/'){ | ||
| s =&s[..s.len() -1]; | ||
| } | ||
| s | ||
| } | ||
| pubfn__repr__(&self) ->String{ |
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.
Does__repr__() also need to be updated?
| @@ -490,7 +514,11 @@ fn unicode_url(lib_url: &Url) -> String { | |||
| s | |||
| } | |||
| _ => s, | |||
| }; | |||
| if remove_trailing_slash && s.ends_with('/') { | |||
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.
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 beremove_trailing_slash: bool = False to match the implementation.
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.
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 withoutpaths. This behavior is being fixed in pydantic-core PR #1719 and willbe 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 Pydanticfixes it (the test will XPASS with xfail_strict=true in pyproject.toml).Ref:pydantic/pydantic-core#1719
Preparation for fixingpydantic/pydantic#7186