Skip to content

gh-68114: Fix handling for removed PyArg_ParseTuple formatters #8204

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 11 commits into from
Apr 23, 2024
Merged

gh-68114: Fix handling for removed PyArg_ParseTuple formatters #8204

merged 11 commits into from
Apr 23, 2024

Conversation

llllllllll
Copy link
Contributor

@llllllllll llllllllll commented Jul 9, 2018 •
edited by bedevere-app bot
Loading

re-opening this as a github pull request @serhiy-storchaka

The only difference from the old patch is that I used blurb to write the news entry.

https://bugs.python.org/issue23926

vstinner
vstinner reviewed Jul 11, 2018
View reviewed changes
@serhiy-storchaka
Copy link
Member

Please merge with master and resolve conflicts with bpo-23927.

ZackerySpytz
ZackerySpytz reviewed Jun 28, 2019
View reviewed changes
@csabella
Copy link
Contributor

csabella commented Feb 6, 2020

@llllllllll, please resolve the merge conflicts and address the code review. Thank you!

MaxwellDupre
MaxwellDupre approved these changes May 18, 2022
View reviewed changes
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.

Test_capi produces:
Ran 40 tests in 7.553s
OK
Looks ok.

iritkatriel
iritkatriel previously requested changes Dec 7, 2022
View reviewed changes
Copy link
Member

@iritkatriel iritkatriel left a comment

Choose a reason for hiding this comment

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

This has merge conflicts now.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

encukou added 8 commits March 18, 2024 17:27
@encukou
Merge in the main branch
cbc1897
@encukou
Merge comments
65aec54
@encukou
Recreate the blurb
6c948fb
@encukou
Fix the test
c2a63a2 
- Don't use removed trivial helper raiseTestError
- Decref kwargs if it's created
- Export the test so that it is run
@encukou
Fix style
4fbed15
@encukou
Ensure the invalid args result in SystemError
3aff049
@encukou
Fix functionality, and add additional test
ae9b1e2 
The original patch didn't call va_arg, resulting in corruption
when 'w*' was used.
@encukou
Unmark 'w#' as supported in test_skipitem_with_suffix
30e7def
@encukou encukou dismissed iritkatriel’s stale review March 19, 2024 09:09

Merge conflicts resolved

@encukou
Copy link
Member

encukou commented Mar 19, 2024

After a few years of unresolved comments I think I can take over the PR.
I've updated the branch, and fixed conflicts and the functionality. But there's enough new content that I'd appreciate another review.

@encukou encukou force-pushed the remove-old-formatters branch from 5f5e116 to 2aee71c Compare March 19, 2024 09:15
@encukou encukou changed the title bpo-23926: Fix handling for removed PyArg_ParseTuple formatters gh-68114: Fix handling for removed PyArg_ParseTuple formatters Apr 23, 2024
@encukou encukou merged commit eb927e9 into python:main Apr 23, 2024
36 checks passed
@llllllllll llllllllll deleted the remove-old-formatters branch November 2, 2024 05:38
@larryhastings larryhastings mentioned this pull request Apr 10, 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 encukou left review comments

@ZackerySpytz ZackerySpytz ZackerySpytz left review comments

@MaxwellDupre MaxwellDupre MaxwellDupre approved these changes

@iritkatriel iritkatriel iritkatriel left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.