Add optional conversions to/from UUID. #4371
Conversation
Taking some liberties here in the meantime and scoping this down. I've limited it now to converting from types that are already Python UUID instances or types that can be extracted to bytes. (With additional validation after). This feels a lot more predictable, IMO Adjusted More tests still pending |
|
As for the "getting consumed" part, that is expected, Uuid is Copy so it will never be moved anywhere. |
🤦 that makes sense |
| fn into_py(self, py: Python<'_>) -> PyObject { | ||
| let rs_bytes = self.into_bytes(); | ||
| let py_bytes = | ||
| unsafe { PyBytes::bound_from_ptr(py, &rs_bytes as *const u8, rs_bytes.len()) }; |
There was a problem hiding this comment.
There's no need for unsafe here, you can just forward to the ToPyObject implementation
| // Not propagating Err up here because the UUID class should be an invariant. | ||
| let uuid_cls = get_uuid_cls(ob.py()).expect("failed to load uuid.UUID"); | ||
| let py_bytes: Bound<'_, PyBytes> = if ob.is_exact_instance(&uuid_cls) { | ||
| ob.getattr(intern!(ob.py(), "bytes"))?.downcast_into()? |
There was a problem hiding this comment.
Pydantic uses the int value to convert to the rust uuid. So dump question: Which one will be faster, int, bytes or does it not matter.
There was a problem hiding this comment.
Ahhh, that's a good point. I had assumed the bytes argument had the lowest overhead, but looking into the UUID class the bytes argument is just a proxy to passing an integer anyways:
if bytes is not None:
if len(bytes) != 16:
raise ValueError('bytes is not a 16-char string')
assert isinstance(bytes, bytes_), repr(bytes)
int = int_.from_bytes(bytes) # big endianhttps://github.com/python/cpython/blob/main/Lib/uuid.py#L192
I'll go ahead and switch to that 👍
|
A little pressed for time last week, but I'll be following up on this shortly |
This is a first pass for #4365. It is based mainly off the current
rust_decimal.There is an open question here about what is the most appropriate set of types to allow automatic conversion from. Currently, I have:
Uuid::try_parse_asciistrthat will be accepted byUuid::try_parseThis does feel a bit too permissive -- but IMO this is more of a maintenance/API question than anything, so @davidhewitt your thoughts would be appreciated here.
Will follow up with the docs if the structure here seems reasonable, and a few more tests.