Skip to content

Implements Comparable for PyDictKeys, PyDictItems#3366

Merged
youknowone merged 3 commits into
RustPython:mainfrom
zetwhite:dict_richcmp
Oct 27, 2021
Merged

Implements Comparable for PyDictKeys, PyDictItems#3366
youknowone merged 3 commits into
RustPython:mainfrom
zetwhite:dict_richcmp

Conversation

@zetwhite

@zetwhite zetwhite commented Oct 24, 2021

Copy link
Copy Markdown
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
Comment thread vm/src/builtins/dict.rs
)
}
ref _set @ PySet => {
let inner = Self::to_set(zelf.to_owned(), vm)?;

@zetwhite zetwhite Oct 24, 2021

Copy link
Copy Markdown
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

@Snowapril Snowapril Oct 24, 2021

Copy link
Copy Markdown
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 😊

@zetwhite zetwhite Oct 24, 2021

Copy link
Copy Markdown
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

Snowapril commented Oct 24, 2021

Copy link
Copy Markdown
Contributor

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

@youknowone

youknowone commented Oct 24, 2021

Copy link
Copy Markdown
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

zetwhite commented Oct 24, 2021

Copy link
Copy Markdown
Contributor Author

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

@DimitrisJim

DimitrisJim commented Oct 24, 2021

Copy link
Copy Markdown
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

@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