-
Notifications
You must be signed in to change notification settings - Fork 307
Preserve order for collections.OrderedDict
#1801
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 9 commits
5980e5b
f5c2042
4f88e03
21d83a3
6e35fe9
ae17586
1b30fbc
be8ef2b
bef11c6
d52d85b
0e40c5c
c0e6b68
a8ffe29
907ed67
9784aa5
e8a8b2a
7767ad5
90b27a1
6cef2dc
84faba3
376a38d
fd28d80
987e3b5
939940d
7f8e658
40050eb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -48,6 +48,35 @@ use super::{ | |
Input, | ||
}; | ||
|
||
static ORDERED_DICT_TYPE: PyOnceLock<Py<PyType>> = PyOnceLock::new(); | ||
|
||
fn get_ordered_dict_type(py: Python<'_>) -> &Bound<'_, PyType> { | ||
ORDERED_DICT_TYPE | ||
.get_or_init(py, || { | ||
py.import("collections") | ||
.and_then(|collections_module| collections_module.getattr("OrderedDict")) | ||
.unwrap() | ||
.extract() | ||
.unwrap() | ||
}) | ||
.bind(py) | ||
} | ||
|
||
fn check_if_ordered_dict(obj: &Bound<'_, PyAny>) -> bool { | ||
if obj.is_exact_instance_of::<PyDict>() { | ||
return false; | ||
} | ||
|
||
if let Ok(type_name) = obj.get_type().name() { | ||
if type_name.to_string() != "OrderedDict" { | ||
return false; | ||
} | ||
} | ||
|
||
let ordered_dict_type = get_ordered_dict_type(obj.py()); | ||
obj.is_instance(ordered_dict_type).unwrap_or(false) | ||
} | ||
|
||
static FRACTION_TYPE: PyOnceLock<Py<PyType>> = PyOnceLock::new(); | ||
|
||
pub fn get_fraction_type(py: Python<'_>) -> &Bound<'_, PyType> { | ||
|
@@ -403,6 +432,14 @@ impl<'py> Input<'py> for Bound<'py, PyAny> { | |
} | ||
|
||
fn lax_dict<'a>(&'a self) -> ValResult<GenericPyMapping<'a, 'py>> { | ||
|
||
if check_if_ordered_dict(self) { | ||
|
||
// OrderedDict is a subclass of dict, but we want to treat it as a mapping to preserve order | ||
if let Ok(mapping) = self.downcast::<PyMapping>() { | ||
return Ok(GenericPyMapping::Mapping(mapping)); | ||
} | ||
} | ||
|
||
if let Ok(dict) = self.downcast::<PyDict>() { | ||
Ok(GenericPyMapping::Dict(dict)) | ||
} else if let Ok(mapping) = self.downcast::<PyMapping>() { | ||
|
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 you will need to fix
strict_dict
too.Probably rather than checking specifically for
OrderedDict
you should returnGenericPyMapping::Mapping
for all cases where it's a subclass of dict.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.
alright I did that. For the
strict_dict
this approach doesn't work since we explicitly check explicitly that an error is thrown if we hand over aMapping
(see here), and it makes sense in strict mode to only allow dicts. To not break code (OrderedDicts were previously working, just didn't keep order), I added an explict check forOrderedDict
in the fashion I previously used forlax_dict
. Let me know what you think here