Repository navigation
Use PyObject_GetItem when converting mappings with from_attributes=False - #1302
princejain756 wants to merge 2 commits into
Conversation
Closes msgspec#1246. In convert_other, when self->from_attributes is false and the input is a mapping, field lookups previously used getattr_then_getitem instead of PyObject_GetItem. Any Struct or dataclass field sharing a name with a mapping method (such as keys, items, values, or get) resolved to the bound method rather than calling __getitem__. Use PyObject_GetItem when self->from_attributes is false, keeping getattr_then_getitem only when self->from_attributes is true.
sobolevn
left a comment
sobolevn
left a comment
There was a problem hiding this comment.
LGTM, but I would like a to have second review :)
|
Haven't looked at it too much yet, but I don't think we're testing this at the moment so: How does this behave with a |
|
With PyObject_GetItem attributes are never consulted, so a test_dataclass_mapping_ignores_attributes covers the dict-subclass version of this; happy to add an explicit |
I think a test case would be good. We're dealing with oddities here, so adding more tests for another oddity, even if just to prevent someone asking that question again, seems reasonable :) |
|
Added the |
Closes #1246.
When converting a non-dict mapping to a
Struct, taggedStructunion, ordataclasswithfrom_attributes=False,convert_otherpreviously setgetter = getattr_then_getitem(introduced in 71c938c). Becausegetattr_then_getitemtriesPyObject_GetAttrfirst, any field sharing a name with an attribute or method on the mapping (keys,items,values,get, etc.) resolved to the bound method instead of looking up the item via__getitem__.This sets
getter = PyObject_GetItemwhenself->from_attributesisfalse, while preservinggetattr_then_getitemfor mappings whenself->from_attributesistrue.Also removes the accidental
(GetAttrOrItemObj, False, False)entry frommapcls_from_attributes_and_array_like(sinceGetAttrOrItemObjstores part of its fields only as attributes) and adds regression tests forStruct, taggedStructunions, anddataclasstargets.