Skip to content

gh-95005: Replace PyAccu with PyUnicodeWriter #95006

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jul 27, 2022

Conversation

aivarsk
Copy link
Contributor

@aivarsk aivarsk commented Jul 19, 2022

PyAccu was still being used in only two places: the JSON encoder and StringIO.
Replace PyAccu with PyUnicodeWriter and remove PyAccu completely.

@aivarsk aivarsk requested a review from a team as a code owner July 19, 2022 12:22
@ghost
Copy link

ghost commented Jul 19, 2022

All commit authors signed the Contributor License Agreement.
CLA signed

@bedevere-bot
Copy link

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@aivarsk aivarsk marked this pull request as draft July 19, 2022 12:25
PyAccu was still being used in only two places: the JSON encoder and StringIO.
Replace PyAccu with PyUnicodeWriter and remove PyAccu completely.
@aivarsk aivarsk marked this pull request as ready for review July 19, 2022 13:28
Copy link
Contributor

@MaxwellDupre MaxwellDupre left a comment

Choose a reason for hiding this comment

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

only 3 tests failed from test suite. Code is good.

PyAccu was still being used in only two places: the JSON encoder and StringIO.
Replace PyAccu with PyUnicodeWriter and remove PyAccu completely.
@corona10 corona10 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 25, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @corona10 for commit a9b95a5 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 25, 2022
@corona10 corona10 self-assigned this Jul 25, 2022
@corona10
Copy link
Member

@MaxwellDupre

only 3 tests failed from test suite. Code is good.

Which test?

@corona10 corona10 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 26, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @corona10 for commit a9b95a5 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 26, 2022
@aivarsk
Copy link
Contributor Author

aivarsk commented Jul 26, 2022

buildbot/AMD64 Debian seems to have a broken compiler or hardware error. The compiler crashed the last time and now it gets SIGBUS while compiling classobject.c which was not modified by the patch.

@MaxwellDupre
Copy link
Contributor

I ran
./python -m test -j0 -W -uall
because of the code coverage and out of >400 tests 3 failed which IMHO was insignificant also those fails did not seem to be related to the fix. Hence, I didn't record them and thought not worth mentioning.

@corona10 corona10 added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 26, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @corona10 for commit a9b95a5 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Jul 26, 2022
@corona10
Copy link
Member

buildbot/AMD64 Debian seems to have a broken compiler or hardware error. The compiler crashed the last time and now it gets SIGBUS while compiling classobject.c which was not modified by the patch.

I sent a mail to the machine owner, seems unrelated to this PR.

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

lgtm

Overall looks good, I just left nit comment.

@corona10 corona10 merged commit 8c88e36 into python:main Jul 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants