[stdlib] Support Dict popitem

[jayzhan211]

close #2355

[gabrieldemarmiesse]

Thanks for the PR, this will be a great addition to Dict! I'm wondering if we can simplify the implementation a bit using previous work.

[gabrieldemarmiesse]

suggestion Now that we have a Tuple type that works with anything, would it make sense to get rid of DictEntry[K, V] and start using Tuple[K, V] ? Of course, changing existing code is outside the scope of this PR, but for this new method popitem() it might make sense to start using Tuple.

[jayzhan211]

Thanks for your review @gabrieldemarmiesse
To me it is better we return DictItem or DictEntry or Dictxxx instead of the broad name like tuple. Unless we are not going to have DictEntry in the future but replaced them all with tuple in the future. 🤔

[gabrieldemarmiesse]

I'm just mentionning this because in python, the thing returned by popitem() or even items() are tuples. See https://docs.python.org/3/library/stdtypes.html#dict.popitem .

I think that we started using DictEntry because Tuple didn't support AnyType yet, it worked with AnyRegType only. Now that Tuple doesn't have this limitation I believe that there is no need for DictEntry. But indeed we can wait for the maintainers to express their views on this. I can open an issue where it can be discussed separately. I'll consider this comment addressed. Thanks!

EDIT: Opened #2554

[gabrieldemarmiesse]

thoughts Would it make sense to wait until your other PR #2340 is merged and then use it? If I'm not mistaken, the implementation could be simplified to:

var key_found: Optional[K] = None
var value_found: Optional[V] = none
for key_value in self.items().__reverse__():
    key_found = key_value.key[]
    value_found = key_value.value[]
if key_found is None:
    raise KeyError
else:
     self.pop(key)
    return key_found, value_found

[jayzhan211]

I think the code here is not quite correct, it will take the first value, but what we need is the last item.

But your overall idea I think is quite nice, I should get the last item and just call pop instead. 👍

[gabrieldemarmiesse]

it will take the first value, but what we need is the last item.

I'm not sure I understand this comment, in the code snippet, isn't using __reverse__ making us start with the last element?

[jayzhan211]

var key_found: Optional[K] = None
var value_found: Optional[V] = none
for key_value in self.items().__reverse__():
    key_found = key_value.key[]
    value_found = key_value.value[]

yes, it starts from the last element, so key_found and value_found is replaced with the first one.

[gabrieldemarmiesse]

oops! My bad! I forgot break in the for loop. Here is the version I was thinking about:

if not self:
    raise "KeyError"

var key_found: Optional[K] = None
var value_found: Optional[V] = None
for key_value in self.items().__reverse__():
    key_found = key_value.key[]
    value_found = key_value.value[]
    break

self.pop(key)
return key_found.get[K](), value_found.get[V]()

What do you think of this version?

EDIT: Changed the exception check to make it simpler.