◐ Shell
reader mode source ↗
Skip to content

bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489)#18608

Merged
vstinner merged 7 commits into
python:masterfrom
shihai1991:bpo_1635741_audioop
Mar 11, 2020
Merged

bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489)#18608
vstinner merged 7 commits into
python:masterfrom
shihai1991:bpo_1635741_audioop

Conversation

@shihai1991

@shihai1991 shihai1991 commented Feb 22, 2020 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member

@codecov

codecov Bot commented Feb 22, 2020 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

Merging #18608 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master   #18608      +/-   ##
==========================================
+ Coverage   82.11%   82.13%   +0.01%     
==========================================
  Files        1956     1955       -1     
  Lines      589425   584605    -4820     
  Branches    44458    44484      +26     
==========================================
- Hits       484018   480164    -3854     
+ Misses      95752    94793     -959     
+ Partials     9655     9648       -7
Impacted Files Coverage Δ
Lib/distutils/tests/test_bdist_rpm.py 30.00% <0.00%> (-65.00%) ⬇️
Lib/distutils/command/bdist_rpm.py 7.63% <0.00%> (-56.88%) ⬇️
Modules/_decimal/libmpdec/umodarith.h 80.76% <0.00%> (-19.24%) ⬇️
Lib/test/test_urllib2net.py 76.92% <0.00%> (-13.85%) ⬇️
Lib/test/test_smtpnet.py 78.57% <0.00%> (-7.15%) ⬇️
Lib/ftplib.py 63.85% <0.00%> (-6.06%) ⬇️
Lib/test/test_ftplib.py 87.11% <0.00%> (-4.72%) ⬇️
Tools/scripts/db2pickle.py 17.82% <0.00%> (-3.97%) ⬇️
Tools/scripts/pickle2db.py 16.98% <0.00%> (-3.78%) ⬇️
Lib/test/test_socket.py 71.94% <0.00%> (-3.77%) ⬇️
... and 352 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c56f8f...5364b74. Read the comment docs.

@shihai1991

shihai1991 commented Feb 22, 2020

Copy link
Copy Markdown
Member Author

Looks there have an potential bug, because running test_audioop test case, the error name not belong to audioop: binascii.Error: Input sample should be longer

@shihai1991 shihai1991 changed the title bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489) Feb 23, 2020
@shihai1991 shihai1991 mentioned this pull request Feb 25, 2020
Merged
@encukou

encukou commented Feb 25, 2020

Copy link
Copy Markdown
Member

I don't have a Windows machine to test this on.
But, I see that the struct name and the macro to get it are both called _audioopstate! I missed that in my earlier review :(

Could you first turn the macro into an inline function, and rename the type (to something like audioop_state)?

@shihai1991

shihai1991 commented Feb 25, 2020

Copy link
Copy Markdown
Member Author

I don't have a Windows machine to test this on.
But, I see that the struct name and the macro to get it are both called _audioopstate! I missed that in my earlier review :(

Could you first turn the macro into an inline function, and rename the type (to something like audioop_state)?

of course, let me try it now ;)

@shihai1991 shihai1991 requested a review from encukou February 25, 2020 17:14
@shihai1991

shihai1991 commented Feb 26, 2020

Copy link
Copy Markdown
Member Author

petr, it's failed again, so I try to find a win env to debug~

@shihai1991 @vstinner
Update Modules/audioop.c …
1a1b56e 
Co-Authored-By: Victor Stinner <vstinner@python.org>
@shihai1991 shihai1991 changed the title [WIP] bpo-1635741: Port audioop extension module to multiphase initialization(PEP 489) Mar 1, 2020
@shihai1991

shihai1991 commented Mar 1, 2020

Copy link
Copy Markdown
Member Author

Oh, gate have passed, thanks, victor.

@vstinner vstinner merged commit 41fbf86 into python:master Mar 11, 2020
30 hidden items Load more…
@kylotan kylotan mannequin mentioned this pull request Sep 19, 2022
Closed
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

@encukou encukou

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@shihai1991 @encukou @vstinner @the-knights-who-say-ni @bedevere-bot