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
youknowone merged 3 commits into from
Oct 27, 2021
Merged

Implements Comparable for PyDictKeys, PyDictItems #3366

youknowone merged 3 commits into from
Oct 27, 2021

Conversation

@zetwhite
Copy link
Contributor

@zetwhite zetwhite commented Oct 24, 2021

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

@zetwhite zetwhite Oct 24, 2021

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

@Snowapril Snowapril Oct 24, 2021

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

@zetwhite zetwhite Oct 24, 2021

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

Snowapril commented Oct 24, 2021

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

@youknowone
Copy link
Member

youknowone commented Oct 24, 2021

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

zetwhite commented Oct 24, 2021

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

@DimitrisJim
Copy link
Member

DimitrisJim commented Oct 24, 2021

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

@youknowone youknowone youknowone approved these changes

+1 more reviewer

@Snowapril Snowapril Snowapril left review comments

Reviewers whose approvals may not affect merge requirements

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