◐ Shell
reader mode source ↗
Skip to content

gh-101696: Make sure immutable types have a valid version tag#101742

Closed
erlend-aasland wants to merge 1 commit into
python:mainfrom
erlend-aasland:versiontag
Closed

gh-101696: Make sure immutable types have a valid version tag#101742
erlend-aasland wants to merge 1 commit into
python:mainfrom
erlend-aasland:versiontag

Conversation

@erlend-aasland

@erlend-aasland erlend-aasland commented Feb 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor

@bedevere-bot bedevere-bot mentioned this pull request Feb 9, 2023
Closed
@erlend-aasland

erlend-aasland commented Feb 9, 2023

Copy link
Copy Markdown
Contributor Author

cc. @markshannon @kumaraditya303

@erlend-aasland

erlend-aasland commented Feb 9, 2023

Copy link
Copy Markdown
Contributor Author

I guess we can skip NEWS, as this is strictly internal.

@erlend-aasland erlend-aasland marked this pull request as ready for review February 9, 2023 17:13
@erlend-aasland erlend-aasland requested review from ericsnowcurrently and removed request for markshannon February 9, 2023 17:14
@kumaraditya303

kumaraditya303 commented Feb 9, 2023

Copy link
Copy Markdown
Contributor

I'll review this by tomorrow.

ericsnowcurrently
ericsnowcurrently requested changes Feb 9, 2023
View reviewed changes

@ericsnowcurrently ericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

LGTM

FWIW, it would be nice if there were a test that covered the original problem from gh-101696 (i.e. fail without the fix and pass with it), whether by crashing or some other means. Would you mind adding something like that in this PR.

@bedevere-bot

bedevere-bot commented Feb 9, 2023

Copy link
Copy Markdown

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

And if you don't make the requested changes, you will be put in the comfy chair!

@erlend-aasland

erlend-aasland commented Feb 9, 2023 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

it would be nice if there were a test that covered the original problem from gh-101696 (i.e. fail without the fix and pass with it), whether by crashing or some other means. Would you mind adding something like that in this PR.

The original problem was uncovered by test_finalize_structseq in test_embed in gh-101304. Without Kumar's fix (d9de079), that particular test segfaulted. With Kumar's fix applied, the test passed; with Kumar's fix reverted and this PR applied, the test also passes.

I could of course try to add a more deterministic test. I'm not quite sure how to provoke that ATM, but I can look into it.

@erlend-aasland

erlend-aasland commented Feb 10, 2023

Copy link
Copy Markdown
Contributor Author

Closing, as per discussion on the issue.

@erlend-aasland erlend-aasland deleted the versiontag branch February 10, 2023 17:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ericsnowcurrently ericsnowcurrently ericsnowcurrently requested changes

@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303

Assignees

No one assigned

Labels

awaiting changes DO-NOT-MERGE skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@erlend-aasland @kumaraditya303 @bedevere-bot @ericsnowcurrently