Skip to content

gh-111262: Make PyDict_Pop() public - #111263

Closed
scoder wants to merge 1 commit into
python:mainfrom
scoder:py313_pydict_capi
Closed

gh-111262: Make PyDict_Pop() public#111263
scoder wants to merge 1 commit into
python:mainfrom
scoder:py313_pydict_capi

Conversation

@scoder

@scoderscoder commented Oct 24, 2023•
edited by github-actions Bot
Loading

Copy link
Copy Markdown
Contributor

See #108449

I kept the internal function since it saves the type check on the fast path and can thus still be used internally.

📚 Documentation preview 📚: https://cpython-previews--111263.org.readthedocs.build/

@bedevere-appbedevere-appBot mentioned this pull request Oct 24, 2023
Closed
@scoderscoder mentioned this pull request Oct 24, 2023
Merged
@scoder

scoder commented Oct 24, 2023

Copy link
Copy Markdown
ContributorAuthor

@vstinner

@brandtbucherbrandtbucher added type-feature A feature request or enhancement interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-C-API labels Oct 24, 2023
vstinner
vstinner reviewed Oct 24, 2023
View reviewed changes

@vstinnervstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The change lacks tests.

Comment threadDoc/c-api/dict.rst
.. versionadded:: 3.4


.. c:function:: PyObject* PyDict_Pop(PyObject *p, PyObject *key, PyObject *defaultobj)

@vstinnervstinnerOct 24, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I would prefer to return -1 on error, 0 if the key doesn't exist, 1 if the key exists.

@pitroupitrouNov 9, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

How would the caller know the popped value then?

pitrou
pitrou reviewed Nov 9, 2023
View reviewed changes
Comment threadObjects/dictobject.c
{
if (!PyDict_Check(dict)) {
PyErr_BadInternalCall();
return -1;

@pitroupitrouNov 9, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should certainly be return NULL?

@vstinnervstinner mentioned this pull request Nov 10, 2023
Closed
@vstinner

vstinner commented Nov 10, 2023

Copy link
Copy Markdown
Member

I created PR #111939 based on this PR: I changed the API and added tests.

@vstinnervstinner mentioned this pull request Nov 13, 2023
Merged
@vstinner

vstinner commented Nov 14, 2023

Copy link
Copy Markdown
Member

I close this PR: #112028 was merged with a different API. Thanks @scoder, I wrote my PR based on yours ;-) And thanks for raising the need for such API.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinnervstinnervstinner left review comments
@pitroupitroupitrou left review comments
@methanemethaneAwaiting requested review from methanemethane is a code owner
@markshannonmarkshannonAwaiting requested review from markshannonmarkshannon is a code owner

Assignees

No one assigned

Labels

awaiting core reviewinterpreter-core(Objects, Python, Grammar, and Parser dirs)topic-C-APItype-featureA feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@scoder@vstinner@pitrou@brandtbucher