bpo-38303: Make audioop extension module PEP-384 compatible by tkieft · Pull Request #16497 · python/cpython · GitHub

https://bugs.python.org/issue38303

Automerge-Triggered-By: @encukou

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

CLA Missing

Our records indicate the following people have not signed the CLA:

@tkieft

For legal reasons we need all the people listed to sign the CLA before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

If you have recently signed the CLA, please wait at least one business day
before our records are updated.

You can check yourself to see if the CLA has been received.

Thanks again for the contribution, we look forward to reviewing it!

Let's remove this line here. This is being discussed in PR16101 and very likely the documentation will change to indicate that it's not needed

Let's follow what most extensions do and return NULL if this fails. An example of this is: _localemodule

Take a look at the following URL and add a NEWS file about your change: https://devguide.python.org/committing/#what-s-new-and-news-entries

Also, change your title to: "Update audioop.c to PEP-384" Like: PR15975

A couple of changes.

Thanks for the review @eduardo-elizondo !

LOL reading my title again, I was looking at the zlib diff and it was late on a friday smh :|

LGTM!

cc: @tiran, @encukou could you take a quick look?

Suggested change

      
            Update audioop extension module to conform to PEP-384
          
            Update audioop extension module to use the stable ABI (PEP-384)

This is a bit of a pet peeve of mine: PEP numbers are only understandable to people who work with that particular PEP. The What's New document deserves better :)

_audioopstate_global should be unnecessary. The module is readily available at all call sites (except for two helper functions whose callers have the module ready).
Using _audioopstate(module) instead should be both faster and more future-proof (see PEP 489, multi-phase init).

For all three functions, the module state may be NULL. Please check the possibility.
(AudioopError may be NULL as well, but the Py_VISIT/Py_CLEAR macros deal with that.)

This is unnecessary; the import mechanism should ensure only one copy of the extension module is imported.

cc @Dormouse759, this might be interesting to you.

Thanks for the detailed review @encukou - made those changes!

@encukou - Mind taking another look?

@tkieft, I hope you don't mind your name in the ACKs. If so, this is good to merge.

Don't mind at all! Thanks!

Sorry, I can't merge this PR. Reason: Required status check "Azure Pipelines PR" is expected..

@zooba What's the best way to get Azure Pipelines to run on this PR?
I see some mailing list discussion on this from May; is "close & re-open" is still the way to go? Perhaps long-term enough to document it in the devguide?

@tkieft: Status check is done, and it's a success ✅ .

Sorry, I can't merge this PR. Reason: Required status check "Azure Pipelines PR" is expected..

@tkieft: Status check is done, and it's a success ✅ .

Sorry, I can't merge this PR. Reason: Required status check "Azure Pipelines PR" is expected..

@tkieft: Status check is done, and it's a success ✅ .

…-16497) https://bugs.python.org/issue38303 Automerge-Triggered-By: @encukou

the-knights-who-say-ni added the CLA not signed label Sep 30, 2019

bedevere-bot added the awaiting review label Sep 30, 2019

eduardo-elizondo reviewed Sep 30, 2019

View reviewed changes

Comment thread Modules/audioop.c Outdated

eduardo-elizondo reviewed Sep 30, 2019

View reviewed changes

eduardo-elizondo suggested changes Sep 30, 2019

View reviewed changes

bedevere-bot added awaiting core review and removed awaiting review labels Sep 30, 2019

the-knights-who-say-ni added CLA signed and removed CLA not signed labels Sep 30, 2019

tkieft changed the title bpo-38303: Make audioop extension module zlib-compatible bpo-38303: Make audioop extension module PEP-384 compatible Sep 30, 2019

eduardo-elizondo approved these changes Oct 1, 2019

View reviewed changes

encukou reviewed Oct 8, 2019

View reviewed changes

Update audioop.c to use the stable ABI (PEP-384 )

44c5624

Add author to ACKS

a8e9c85

encukou approved these changes Oct 22, 2019

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting core review labels Oct 22, 2019

encukou added the 🤖 automerge label Oct 22, 2019

miss-islington merged commit f548a3e into python:master Oct 22, 2019

bedevere-bot removed the awaiting merge label Oct 22, 2019

tkieft deleted the pep384/audioop branch October 22, 2019 13:10

encukou mentioned this pull request Oct 22, 2019

bpo-37645: add new function _PyObject_FunctionStr() #14890

Merged

jacobneiltaylor pushed a commit to jacobneiltaylor/cpython that referenced this pull request Dec 5, 2019

bpo-38303: Make audioop extension module PEP-384 compatible (pythonGH…

eef0831

…-16497) https://bugs.python.org/issue38303 Automerge-Triggered-By: @encukou

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020

bpo-38303: Make audioop extension module PEP-384 compatible (pythonGH…

c084fb2

…-16497) https://bugs.python.org/issue38303 Automerge-Triggered-By: @encukou

	Update audioop extension module to conform to PEP-384
	Update audioop extension module to use the stable ABI (PEP-384)

bpo-38303: Make audioop extension module PEP-384 compatible by tkieft · Pull Request #16497 · python/cpython · GitHub

Repository navigation

bpo-38303: Make audioop extension module PEP-384 compatible - #16497

bpo-38303: Make audioop extension module PEP-384 compatible#16497miss-islington merged 2 commits intopython:masterpython/cpython:masterfrom tkieft:pep384/audioopCopy head branch name to clipboard

Conversation

tkieft commented Sep 30, 2019 • edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

the-knights-who-say-ni commented Sep 30, 2019

CLA Missing

Uh oh!

eduardo-elizondo Sep 30, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eduardo-elizondo Sep 30, 2019

Choose a reason for hiding this comment

Uh oh!

eduardo-elizondo commented Sep 30, 2019

Uh oh!

eduardo-elizondo left a comment

Choose a reason for hiding this comment

Uh oh!

tkieft commented Sep 30, 2019

Uh oh!

tkieft commented Sep 30, 2019

Uh oh!

eduardo-elizondo left a comment

Choose a reason for hiding this comment

Uh oh!

eduardo-elizondo commented Oct 1, 2019

Uh oh!

encukou Oct 8, 2019

Choose a reason for hiding this comment

Uh oh!

encukou Oct 8, 2019

Choose a reason for hiding this comment

Uh oh!

encukou Oct 8, 2019

Choose a reason for hiding this comment

Uh oh!

encukou Oct 8, 2019

Choose a reason for hiding this comment

Uh oh!

encukou commented Oct 8, 2019

Uh oh!

tkieft commented Oct 10, 2019

Uh oh!

tkieft commented Oct 21, 2019

Uh oh!

encukou left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tkieft commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

encukou commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

miss-islington commented Oct 22, 2019

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

bpo-38303: Make audioop extension module PEP-384 compatible#16497
miss-islington merged 2 commits into
python:masterfrom
tkieft:pep384/audioop

tkieft commented Sep 30, 2019 •
edited by miss-islington
Loading

eduardo-elizondo Sep 30, 2019 •
edited
Loading

encukou left a comment •
edited
Loading