implement Input for str
#1229
implement Input for str
#1229
Conversation
CodSpeed Performance ReportMerging #1229 will not alter performanceComparing Summary
|
| /// parse a string as an int, `input` is required here to get lifetimes to match up | ||
| /// | ||
| fn _parse_str<'s>(_input: &'s impl Input<'s>, str: &str, len: usize) -> Option<EitherInt<'s>> { | ||
| fn _parse_str<'py>(_input: &(impl Input<'py> + ?Sized), str: &str, len: usize) -> Option<EitherInt<'py>> { |
There was a problem hiding this comment.
There's a lot of &(impl Input<'py> + ?Sized), can you explain better what it does/why we're using it?
There was a problem hiding this comment.
impl Trait by default is a type which implements Sized, unfortunately str does not implement Sized
So for &str to be compatible with &impl Trait we need to go for &(impl Trait + ?Sized) to say we're ok with no sized requirement.
So removing Input: Sized from the trait definition is necessary to allow str, and then for impl Trait everywhere we have to add ?Sized opt out.
It's a bit ugly, but I think if we ever go for iterative JSON parsing then the whole Input trait needs to be reworked to carry state somehow, so I sort of felt that we could live with this pain in the meanwhile.
Also where possible I went for impl ToErrorValue. &str implements Sized, as it's just a reference, even if str doesn't. So impl ToErrorValue is much nicer to type compared to &(impl Input<'py> + ?Sized) where relevant. 😂
There was a problem hiding this comment.
Makes sense, thanks for the detail.
Original-commit-link: pydantic/pydantic-core@352d40f
Change Summary
This is on the way to supporting https://github.com/pydantic/jiter/pull/63/files
Because that PR changes the input type for the JSON data to
Cow<'_, str>, we run into trouble with the existing implementation ofInput for Stringnot being sufficient to cover theBorrowed(i.e.str) variant of theCow.After lots of wrestling with options I decided this is the right implementation, as it makes this as reusable as possible and helps to push further with the ideas in #1227 to unpick the two lifetimes.
I want to get this reviewed & merged before the final
Cowsupport PR because it changes a lot about how the lifetimes are written without any of the other logical changes related to theCow, which will be more interesting to review on their own.Related issue number
N/A
Checklist
pydantic-core(except for expected changes)