[Feature] OrderedDict.move_to_end

[elpekenin]

Closes #4408

Adds the method in the title. First time i write a C-Python binding, please let me know if anything should be changed

• Should maybe remove the doctstring to follow other funcs not having it on the file
• Is there a way to extend existing array instead of duplicating? Could save a bit of space

---

My little test:

Adafruit CircuitPython 8.2.0-47-g8050b07a6-dirty on 2023-07-30; WeAct ESP32S3-B-N16R8 with ESP32S3R8

>>> from collections import OrderedDict
>>> d = OrderedDict.fromkeys("abcd")
>>> print(d)
OrderedDict({'a': None, 'b': None, 'c': None, 'd': None})
>>> d.move_to_end("a")
>>> print(d)
OrderedDict({'b': None, 'c': None, 'd': None, 'a': None})
>>> d.move_to_end("d", False)
>>> print(d)
OrderedDict({'d': None, 'b': None, 'c': None, 'a': None})
>>> d.move_to_end("b", last=False)
>>> print(d)
OrderedDict({'b': None, 'd': None, 'c': None, 'a': None})

[jepler]

Unfortunately, this change is overflowing the available flash size on a few builds :-/ there's no easy solution for this, the resolution may be to choose not to include it, or to disable some other feature on the failing board.

[elpekenin]

Yeah, should i add a new #define + #ifdef to control whether the API's code gets added?

Could also add the method to dict's table, but this breaks worse than fixes:

• i dont think the method exists on regular dicts
• would only save a few bytes (not duplicating a couple of pointers and whatnot across two tables)

[dhalbert]

Yeah, should i add a new #define + #ifdef to control whether the API's code gets added?

Yup. We use #if instead of #ifdef so we make sure that the setting has a defined value. There are many examples of that in various places.

Instead of making yet another flag, there is a kitchen-sink one called MICROPY_CPYTHON_COMPAT which turns on various extra features. I think this feature should be controlled by that for now, so that it's not added for the smallest builds. In the long run I wanted to refactor that flag into separate flags, but in the short run that would be an easy flag to use.

However, the boards that are overflowing I don't think would be fixed by that. We need to look at the SAMD51 boards that are close to overflowing and come up with a general strategy to shrink them. @jepler started to do this for some other boards. For instance, we may be able to shrink ulab significantly. So let's hold this PR up for a bit while we look jharder. Since this would be a 9.0.0 feature, merging is not urgent.

[elpekenin]

No worries, we're not in a rush :D

Will add the new #if soon, and make the #else do ordered_dict_locals_dict = dict_locals_dict

Let me know if any code should be re-written for readability/matching codestyle of the repo

[elpekenin]

Just in case: Dont merge yet, i think key is positional-only right now (forgot its QSTR on the structure which parses args)

EDIT: Should be good to go now.

[dhalbert]

Could you add some tests to tests/basic? There are some OrderedDict tests there already. Add a new one. See tests/README for whether you need a .exp file or not. The testing procedure runs the test in both CircuitPython and CPython (regular Python). If they are the same the test passes. If the tests might not be the same, then a .exp file is provided that is the result one would expect from running the test in CircuitPython.

[dhalbert]

Here are the test failure details:

 True
 abcdefghij
 abcdefghij
-odict_keys(['a', 'b'])
-odict_keys(['b', 'a'])
-odict_keys(['a', 'b'])
-odict_keys(['b', 'a'])
-odict_keys(['a', 'b'])
-odict_keys(['b', 'a'])
-odict_keys(['a', 'b'])
+dict_keys(['a', 'b'])
+dict_keys(['b', 'a'])
+dict_keys(['a', 'b'])
+dict_keys(['b', 'a'])
+dict_keys(['a', 'b'])
+dict_keys(['b', 'a'])
+dict_keys(['a', 'b'])

FAILURE /home/runner/work/circuitpython/circuitpython/tests/results/basics_ordereddict1.py

Looks like it changed from printing odict_keys to dict_keys.

See https://github.com/adafruit/circuitpython/actions/runs/5744164923/job/15569982403?pr=8234, and then click "Print failure info" to see the details I pasted above.

[elpekenin]

Should i have that many tests, for every combination of positional and keyword usage (made that to confirm i made things correctly), or would it be better to just check moving either way?

[dhalbert]

Should i have that many tests, for every combination of positional and keyword usage (made that to confirm i made things correctly), or would it be better to just check moving either way?

I think the main thing is to test the moving and edge cases (e.g. on an empty dict, key not found, etc.) not so much the arg parsing. It doesn't hurt but the testing is more about the functionality than the arg parsing. Also test arg validation maybe: suppose you pass a non-bool for the bool arg.

[elpekenin]

Tests are now checking (all?) edge cases + are successful, however flash is still too large...

[jepler]

I wrote #8258 which also implements move_to_end, but at smaller flash cost. Thank you for the original implementation!

[dhalbert]

Closed in favor of #8258.