Skip to content
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

gh-117398: Add datetime C-API type check test for subinterpreters #119604

Merged
merged 40 commits into from
Jun 13, 2024
Merged

gh-117398: Add datetime C-API type check test for subinterpreters #119604

merged 40 commits into from
Jun 13, 2024

Conversation

neonene
Copy link
Contributor

@neonene neonene commented May 27, 2024 •
edited
Loading

Check if the DateType C-API type matches the datetime.date type on main and shared/isolated subinterpreters.

@ericsnowcurrently Please review if this usage is not invalid.

@bedevere-app bedevere-app bot added tests Tests in the Lib/test dir awaiting review labels May 27, 2024
@bedevere-app bedevere-app bot mentioned this pull request May 27, 2024
Closed
ericsnowcurrently
ericsnowcurrently requested changes May 27, 2024
View reviewed changes
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

The test makes sense. Thanks for adding it. I have one question.

Modules/_testmultiphase.c Outdated Show resolved Hide resolved
@bedevere-app
Copy link

bedevere-app bot commented May 27, 2024

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.

@neonene
Copy link
Contributor Author

neonene commented May 28, 2024

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented May 28, 2024

Thanks for making the requested changes!

@ericsnowcurrently: please review the changes made to this pull request.

@neonene neonene changed the title gh-117398: Add test for datetime C API type check gh-117398: Add C-API test for datetime static type check May 28, 2024
@ericsnowcurrently
Copy link
Member

It looks like part of your PR was removed by mistake. I thought the new test in test_capi.test_misc would still be there.

@neonene neonene changed the title gh-117398: Add C-API test for datetime static type check gh-117398: Add datetime C-API type check test for subinterpreters May 29, 2024
ericsnowcurrently
ericsnowcurrently requested changes Jun 13, 2024
View reviewed changes
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

Thanks for moving that test! There's just one thing to address and then this PR should be ready to merge. Thanks for your patience.

Lib/test/support/__init__.py Outdated Show resolved Hide resolved
@bedevere-app
Copy link

bedevere-app bot commented Jun 13, 2024

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.

@neonene
Copy link
Contributor Author

neonene commented Jun 13, 2024

I have made the requested changes; please review again.

@bedevere-app
Copy link

bedevere-app bot commented Jun 13, 2024

Thanks for making the requested changes!

@ericsnowcurrently: please review the changes made to this pull request.

ericsnowcurrently
ericsnowcurrently approved these changes Jun 13, 2024
View reviewed changes
Copy link
Member

@ericsnowcurrently ericsnowcurrently left a comment

Choose a reason for hiding this comment

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

LGTM

@ericsnowcurrently ericsnowcurrently merged commit 50a3895 into python:main Jun 13, 2024
35 of 38 checks passed
@miss-islington-app
Copy link

Thanks @neonene for the PR, and @ericsnowcurrently for merging it 🌮🎉.. I'm working now to backport this PR to: 3.13.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jun 13, 2024
@neonene @miss-islington
pythongh-117398: Add datetime C-API type check test for subinterprete…
8fb3ea8 
…rs (pythongh-119604)

Check if the DateTime C-API type matches the datetime.date type on main and shared/isolated subinterpreters.
(cherry picked from commit 50a3895)

Co-authored-by: neonene <[email protected]>
@bedevere-app
Copy link

bedevere-app bot commented Jun 13, 2024

GH-120463 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jun 13, 2024
@ericsnowcurrently
Copy link
Member

Thanks again for all your work on this, @neonene!

@neonene neonene deleted the capi_type_check branch June 13, 2024 18:10
ericsnowcurrently pushed a commit that referenced this pull request Jun 13, 2024
@miss-islington @neonene
[3.13] gh-117398: Add datetime C-API type check test for subinterpret…
15c3d00 
…ers (gh-120463)

Check if the DateTime C-API type matches the datetime.date type on main and shared/isolated subinterpreters.

(cherry picked from commit 50a3895, AKA gh-119604)

Co-authored-by: neonene <[email protected]>
@freakboy3742 freakboy3742 mentioned this pull request Jun 14, 2024
Merged
mrahtz pushed a commit to mrahtz/cpython that referenced this pull request Jun 30, 2024
@neonene @mrahtz
pythongh-117398: Add datetime C-API type check test for subinterprete…
2129fa4 
…rs (pythongh-119604)

Check if the DateTime C-API type matches the datetime.date type on main and shared/isolated subinterpreters.
@neonene neonene mentioned this pull request Jul 10, 2024
Merged
noahbkim pushed a commit to hudson-trading/cpython that referenced this pull request Jul 11, 2024
@neonene @noahbkim
pythongh-117398: Add datetime C-API type check test for subinterprete…
78a5805 
…rs (pythongh-119604)

Check if the DateTime C-API type matches the datetime.date type on main and shared/isolated subinterpreters.
estyxx pushed a commit to estyxx/cpython that referenced this pull request Jul 17, 2024
@neonene @estyxx
pythongh-117398: Add datetime C-API type check test for subinterprete…
6285938 
…rs (pythongh-119604)

Check if the DateTime C-API type matches the datetime.date type on main and shared/isolated subinterpreters.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently approved these changes

@markshannon markshannon Awaiting requested review from markshannon

@gvanrossum gvanrossum Awaiting requested review from gvanrossum

@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner

@abalkin abalkin Awaiting requested review from abalkin abalkin is a code owner

Assignees
No one assigned
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@neonene @ericsnowcurrently @encukou