-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
[stdlib] Support Dict popitem #2542
Conversation
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
Signed-off-by: jayzhan211 <jayzhan211@gmail.com>
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.
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.
@@ -644,6 +644,44 @@ struct Dict[K: KeyElement, V: CollectionElement]( | |||
return default.value()[] | |||
raise "KeyError" | |||
|
|||
fn popitem(inout self) raises -> DictEntry[K, V]: |
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.
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.
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.
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. 🤔
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'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
stdlib/src/collections/dict.mojo
Outdated
"KeyError" if the dictionary is empty. | ||
""" | ||
# Get the last key so we can get the slot and set it to REMOVED | ||
for key in Self.__reversed__(self): |
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.
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
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 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. 👍
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.
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?
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.
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.
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.
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.
close #2355