-
Notifications
You must be signed in to change notification settings - Fork 784
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
move PyDict::get_item_with_error to PyDict::get_item #3330
Conversation
8924da6
to
f6ff2ae
Compare
This is obviously quite an irritating change for users because it changes the return type from I still defend the change as worthwhile given it nudges them onto using a "correct" API instead of unintentionally ignoring inner errors. |
f6ff2ae
to
a373a73
Compare
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.
+1 for this change.
It'd be nice to flesh out the docs. e.g. to answer questions like
- is it OK to unwrap or
unwrap_or(None)
? - should I just bubble up the error?
- when can this return an error?
608a45f
to
2cc5452
Compare
Sorry for the long delay here. I've added some detail to the documentation to cover the points raised above, they were all good suggestions 👍 |
2cc5452
to
a6a1b20
Compare
a6a1b20
to
8ddfef0
Compare
As this has had an approval and I think any further changes which might be requested from further reviews would be docs rather than API, I'm going to merge this now so that I can start building an 0.20 release branch. |
As per capi-workgroup/problems#51 the recommendation of the Python core devs is to avoid
PyDict_GetItem
because it can hide arbitrary exceptions.Accordingly I've moved the
PyDict::get_item_with_error
to just becomePyDict::get_item
, and deprecated theget_item_with_error
name.