Repository navigation
Implement IncrementalNewlineDecoder in rust - #5443
Conversation
1d8c3e8 to
fbd0c7a
Compare
youknowone
left a comment
youknowone
left a comment
There was a problem hiding this comment.
Great, finally we have rust implementation of newline decoder.
Please check comment if you need TryFromBorrowedObject for PyObject.
|
|
||
| let name = field.ident.as_ref(); | ||
| let name_string = name.map(Ident::to_string); | ||
| let name_string = name.map(|ident| ident.unraw().to_string()); |
| } | ||
| } | ||
|
|
||
| // TODO: figure out a way to let PyObjectRef implement TryFromBorrowedObject, and |
There was a problem hiding this comment.
PyObjectRef cannot, but PyObject can. Since PyObject can always be borrowed from PyObjectRef, I expect it has same effect.
impl<'a> TryFromBorrowedObject<'a> for &'a PyObject {
fn try_from_borrowed_object(_vm: &VirtualMachine, obj: &'a PyObject) -> PyResult<Self> {
Ok(obj)
}
}There was a problem hiding this comment.
That is possible, but not allowing PyObjectRef to be extracted isn't ideal, and I'd rather allow that and disallow &PyObject than the other way around.
There was a problem hiding this comment.
Could you explain more about it?
For me, it doesn't seem possible without increasing refcount. And increasing a refcount is something not expected from TryFromBorrowedObject
There was a problem hiding this comment.
Oh, I didn't realize that was part of the contract for TryFromBorrowedObject. It seems useful to me to have some way of generalizing over a type that can be extracted from a borrowed pyobj, but doesn't necessarily have to borrow from it - like serde's Deserialize vs DeserializeOwned. Like this case - the T in extract_tuple() should be able to borrow from the pyobjectref in the tuple, but there's no reason we should enforce it - a (PyObjectRef,) is a perfectly reasonable thing to put there.
There was a problem hiding this comment.
Thanks, I think I am going to catch up.
No description provided.