Skip to content

Implements Comparable for PyDictKeys, PyDictItems #3366

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Oct 27, 2021
Merged

Implements Comparable for PyDictKeys, PyDictItems #3366

merged 3 commits into from
Oct 27, 2021

Conversation

zetwhite
Copy link
Contributor

For #3298

I moved Comparable Implements in dict_view macro to outside,
because only PyDictItems, PyDictKeys need to implement Comparable, not PyDictItems.
(check in https://github.com/python/cpython/blob/9942f42a93ccda047fd3558c47b822e99afe10c0/Objects/dictobject.c#L4703, https://github.com/python/cpython/blob/9942f42a93ccda047fd3558c47b822e99afe10c0/Objects/dictobject.c#L4809)

After #3316(PySequence Layer) is merged, Comparable implementation can be simpler.
But now, I just let it compare differently depending on the type.

before

>>>>> d = {1 : 1, 2 : 2}
>>>>> d.keys() == set([1, 2])
False
>>>>> d.keys() <= set([1, 2])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: '<=' not supported between instances of 'dict_keys' and 'set'
>>>>> d.items() == set([(1, 1), (2, 2)])
False
>>>>> d.items() <= set([(1, 1), (2, 2)])
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
TypeError: '<=' not supported between instances of 'dict_items' and 'set'

after

>>>>> d = {1 : 1, 2 : 2}
>>>>> d.keys() == set([1, 2])
True
>>>>> d.keys() <= set([1, 2])
True
>>>>> d.items() == set([(1, 1), (2, 2)])
True
>>>>> d.items() <= set([(1, 1), (2, 2)])
True

zetwhite
zetwhite commented Oct 24, 2021
View reviewed changes
vm/src/builtins/dict.rs
)
}
ref _set @ PySet => {
let inner = Self::to_set(zelf.to_owned(), vm)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure this line is fine or not.
If you have any idea, please comment

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

to_set make copy of all elements. Iterating all elements in zelf and using PySet::contains may make sense like all_contained_in in cpython. Although we dont have PySequence_Contains, we have PySet type we can use it 😊

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay. I'll check and fix it. 👍 😄

@Snowapril
Copy link
Contributor

there are unexpected success cases in test_dictviews.py (test_dict_items, test_dict_keys)

@youknowone
Copy link
Member

How about putting cmp to ViewSetOps to reuse it from items and keys?

@DimitrisJim DimitrisJim linked an issue Oct 24, 2021 that may be closed by this pull request
Handling sets in cmp for dict view types #3298
Closed
@zetwhite
Copy link
Contributor Author

@youknowone
Do you mean to move only three lines? Or the whole cmp function?
a3fed90#diff-aec5b89f34446c0d6c48bc4c4ddd59a83757612cc4b9705186d967c7234f7984R990-R992

@DimitrisJim
Copy link
Member

I think you can move the whole cmp implementation. It seems identical for both items and keys.

@youknowone youknowone merged commit 35e56ef into RustPython:main Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Snowapril Snowapril Snowapril left review comments

@youknowone youknowone youknowone approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Handling sets in cmp for dict view types
4 participants
@zetwhite @Snowapril @youknowone @DimitrisJim