GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — gh-111262: Make PyDict_Pop() public by scoder · Pull Request #111263 · python/cpython

Conversation

@scoder

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

Uh oh!

There was an error while loading. Please reload this page.

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.

  • Issue: Make _PyDict_Pop() public #111262

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
pitrou reacted with thumbs up emoji
All reactions
  • 👍 1 reaction
@scoder
scoder requested review from markshannon and methane as code owners October 24, 2023 14:01
@bedevere-app bedevere-app Bot mentioned this pull request Oct 24, 2023
Make _PyDict_Pop() public #111262
Closed
@bedevere-app bedevere-app Bot added the awaiting core review label Oct 24, 2023
@scoder
scoder force-pushed the py313_pydict_capi branch from 5674aa3 to 9c8539b Compare October 24, 2023 14:04
@scoder
pythongh-106320: Make some PyDict C-API functions public that should …
16a95d4 
…have been public right away.

See python#108449
@scoder
scoder force-pushed the py313_pydict_capi branch from 9c8539b to 16a95d4 Compare October 24, 2023 14:05
@scoder scoder mentioned this pull request Oct 24, 2023
Adapt to C-API changes in CPython 3.13 cython/cython#5767
Merged
@scoder

scoder commented Oct 24, 2023

Copy link
Copy Markdown
Contributor Author

@vstinner
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@brandtbucher brandtbucher 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

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

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.

The change lacks tests.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Doc/c-api/dict.rst
.. versionadded:: 3.4


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

@vstinner vstinner Oct 24, 2023

Copy link
Copy Markdown
Member

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.

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

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@pitrou pitrou Nov 9, 2023

Copy link
Copy Markdown
Member

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.

How would the caller know the popped value then?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
pitrou
pitrou reviewed Nov 9, 2023
View reviewed changes
Comment thread Objects/dictobject.c
{
if (!PyDict_Check(dict)) {
PyErr_BadInternalCall();
return -1;

@pitrou pitrou Nov 9, 2023

Copy link
Copy Markdown
Member

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.

Should certainly be return NULL?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@vstinner vstinner mentioned this pull request Nov 10, 2023
gh-111262: Add PyDict_Pop() function [with default value] #111939
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner mentioned this pull request Nov 13, 2023
gh-111262: Add PyDict_Pop() function [without default value nor KeyError] #112028
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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@vstinner vstinner closed this Nov 14, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments
@pitrou pitrou pitrou left review comments
@methane methane Awaiting requested review from methane methane is a code owner
@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting core review interpreter-core (Objects, Python, Grammar, and Parser dirs) topic-C-API type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@scoder @vstinner @pitrou @brandtbucher
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.