-
Notifications
You must be signed in to change notification settings - Fork 782
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
types: add PyMapping #1844
types: add PyMapping #1844
Conversation
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.
As I proposed in #1845 for sequences, the API here should mirror PyDict as closely as possible.
src/types/mapping.rs
Outdated
K: ToBorrowedObject, | ||
{ | ||
let r = key.with_borrowed_ptr(self.py(), |ptr| unsafe { | ||
ffi::PyMapping_HasKey(self.as_ptr(), ptr) |
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.
If we use HasKey
which suppresses exceptions, the method can return just bool
?
Otherwise, use GetItem
internally to propagate the exceptions...
👍 will update this to follow what you've done in #1849 but for |
fc15d42
to
d22d623
Compare
Updated to contain the other relevant In the end I couldn't come up with a good decision for So for now I left |
d22d623
to
d46209c
Compare
Any objections if I merge this? |
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.
Could add an entry in guide/src/conversions/tables.md
I guess.
d46209c
to
d929916
Compare
👍 good idea, done! |
Is there a reason that |
See #1844 (comment) I just couldn't make an obvious decision on the semantics of |
Don't we have a method for the |
Not that I could find? |
Adds a
PyMapping
type - similar toPySequence
, but specially to interact with the mapping protocol.It doesn't have
get_item
,set_item
, anddel_item
methods because they're already present because ofDeref
toPyAny
. However it contains a few other helpful pieces such as.contains_key()
,.keys()
,.values()
, and.items()
.Seemed like it might be useful for davidhewitt/pythonize#16 (cc @i1i1)
WIP - needs tests.