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-102192: deprecate _PyErr_ChainExceptions by iritkatriel · Pull Request #102935 · python/cpython

Conversation

@iritkatriel

@iritkatriel iritkatriel commented Mar 22, 2023 •
edited by bedevere-bot
Loading

Uh oh!

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

Copy link
Copy Markdown
Member
  • Issue: Replace Fetch/Restore etc by the new exception APIs #102192

Sorry, something went wrong.

Uh oh!

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

All reactions
@iritkatriel
pythongh-102192: remove unused _PyErr_ChainExceptions
de98c08
@bedevere-bot bedevere-bot mentioned this pull request Mar 22, 2023
Replace Fetch/Restore etc by the new exception APIs #102192
Closed
@bedevere-bot bedevere-bot added the awaiting core review label Mar 22, 2023
@iritkatriel iritkatriel added skip news interpreter-core (Objects, Python, Grammar, and Parser dirs) labels Mar 22, 2023
@iritkatriel
iritkatriel requested a review from erlend-aasland March 30, 2023 16:41
@erlend-aasland

erlend-aasland commented Mar 31, 2023

Copy link
Copy Markdown
Contributor

I'm not sure we can simply remove this from the C API, as it is unfortunately exposed via Python.h:

$ echo '#include "Python.h"' | gcc -E $(./python.exe ./python-config.py --includes) - | grep _PyErr_ChainExceptions
__attribute__ ((visibility ("default"))) void _PyErr_ChainExceptions(PyObject *, PyObject *, PyObject *);
__attribute__ ((visibility ("default"))) void _PyErr_ChainExceptions1(PyObject *);

cc. @encukou

All reactions

Sorry, something went wrong.

Uh oh!

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

markshannon
markshannon requested changes Mar 31, 2023
View reviewed changes

@markshannon markshannon 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 code looks fine, but it will need documenting.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Include/cpython/pyerrors.h

/* Context manipulation (PEP 3134) */

PyAPI_FUNC(void) _PyErr_ChainExceptions(PyObject *, PyObject *, PyObject *);

@markshannon markshannon Mar 31, 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.

This is marked as an API function, so it will need to be added to the "porting to 3.12" section and any other relevant sections in the "what's new".

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot removed the awaiting core review label Mar 31, 2023
@bedevere-bot

bedevere-bot commented Mar 31, 2023

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

All reactions

Sorry, something went wrong.

Uh oh!

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

@bedevere-bot bedevere-bot added the awaiting changes label Mar 31, 2023
@markshannon

markshannon commented Mar 31, 2023

Copy link
Copy Markdown
Member

I'm not sure we can simply remove this from the C API, as it is unfortunately exposed via Python.h:

It is prefixed with an underscore so we can, in theory, remove it. But we will need to document how to upgrade to 3.12.
We could leave _PyErr_ChainExceptions in as a deprecated shim that calls _PyErr_ChainExceptions1 for a couple of releases, if we think removing it will cause breakages.

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel

iritkatriel commented Mar 31, 2023

Copy link
Copy Markdown
Member Author

We could leave _PyErr_ChainExceptions in as a deprecated shim that calls _PyErr_ChainExceptions1 for a couple of releases

Currently it normalises the exception so just calling _PyErr_ChainExceptions1 with the second arg would be a change in semantics.

erlend-aasland reacted with thumbs up emoji
All reactions
  • 👍 1 reaction

Sorry, something went wrong.

Uh oh!

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

@iritkatriel
Revert "pythongh-102192: remove unused _PyErr_ChainExceptions"
c1261c7
This reverts commit de98c08.
@erlend-aasland

erlend-aasland commented Mar 31, 2023

Copy link
Copy Markdown
Contributor

Since it is exposed via Python.h, someone is bound to be using it, so we cannot just remove it. I'd say we mark it as deprecated using the Py_DEPRECATED macro. There's practically no maintenance overhead in keeping it around.

BTW, we should consider unexposing the new API (_PyErr_ChainExceptions1) by moving it to Include/internal before 3.12 is frozen.

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel iritkatriel changed the title gh-102192: remove unused _PyErr_ChainExceptions gh-102192: deprecate _PyErr_ChainExceptions Mar 31, 2023
@iritkatriel

iritkatriel commented Mar 31, 2023 •
edited
Loading

Uh oh!

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

Copy link
Copy Markdown
Member Author

I searched on GitHub and found quite a few usages. So I agree with just deprecating it for now.

Not sure it's reasonable to not expose the alternative though, given people are using it.

All reactions

Sorry, something went wrong.

Uh oh!

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

@iritkatriel
deprecate it
ca61240
@iritkatriel iritkatriel removed the skip news label Mar 31, 2023
iritkatriel added 3 commits March 31, 2023 12:22
@iritkatriel
add news
4fc222b
@iritkatriel
whitespace
3cbe6be
@iritkatriel
hopefully fix news build
5765ba2
@erlend-aasland

erlend-aasland commented Mar 31, 2023

Copy link
Copy Markdown
Contributor

Not sure it's reasonable to not expose the alternative though, given people are using it.

Since people are using it, we should consider dropping the underscore, IMO.

All reactions

Sorry, something went wrong.

Uh oh!

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

erlend-aasland
erlend-aasland approved these changes Apr 1, 2023
View reviewed changes

@erlend-aasland erlend-aasland left a comment

Copy link
Copy Markdown
Contributor

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.

LGTM.

We should consider making the new API official and documenting it, in a follow-up PR.

Sorry, something went wrong.

Uh oh!

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

All reactions
Comment thread Doc/whatsnew/3.12.rst Outdated
(Contributed by Mark Shannon in :gh:`101578`.)

* Add ``_PyErr_ChainExceptions1``, which takes an exception instance,
to replace the legacy-api ``_PyErr_ChainExceptions``, which is now

@erlend-aasland erlend-aasland Apr 1, 2023

Copy link
Copy Markdown
Contributor

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.

Nit: api => API

Sorry, something went wrong.

Uh oh!

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

All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels Apr 1, 2023
iritkatriel
iritkatriel commented Apr 1, 2023
View reviewed changes
Comment thread Doc/whatsnew/3.12.rst Resolved

Uh oh!

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

@iritkatriel
api->API
9145555
@iritkatriel
iritkatriel merged commit 06249ec into python:main Apr 1, 2023
@bedevere-bot bedevere-bot removed the awaiting merge label Apr 1, 2023
@iritkatriel
iritkatriel deleted the remove-unused branch April 3, 2023 17:41
gaogaotiantian pushed a commit to gaogaotiantian/cpython that referenced this pull request Apr 8, 2023
@iritkatriel @gaogaotiantian
pythongh-102192: deprecate _PyErr_ChainExceptions (python#102935)
ab883c3
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
@iritkatriel @warsaw
pythongh-102192: deprecate _PyErr_ChainExceptions (python#102935)
57a254d
JukkaL added a commit to python/mypy that referenced this pull request Jun 19, 2023
@JukkaL
[mypyc] Don't use _PyErr_ChainExceptions on 3.12, since it's deprecated
052e61b
See python/cpython#102935.
@JukkaL JukkaL mentioned this pull request Jun 19, 2023
[mypyc] Don't use _PyErr_ChainExceptions on 3.12, since it's deprecated python/mypy#15468
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@markshannon markshannon markshannon requested changes
@erlend-aasland erlend-aasland erlend-aasland approved these changes

Uh oh!

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

Assignees

No one assigned

Labels

interpreter-core (Objects, Python, Grammar, and Parser dirs)

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

@iritkatriel @erlend-aasland @bedevere-bot @markshannon
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.