Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

gh-99138: Isolate _zoneinfo#99218

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
erlend-aasland merged 30 commits intopython:mainfromerlend-aasland:isolate-zoneinfo
Feb 15, 2023

Conversation

@erlend-aasland
Copy link
Contributor

@erlend-aaslanderlend-aasland commentedNov 7, 2022•
edited by bedevere-bot
Loading

@erlend-aaslanderlend-aasland changed the titlegh-99138: Isolate zoneinfogh-99138: Isolate _zoneinfoNov 7, 2022
@erlend-aasland
Copy link
ContributorAuthor

erlend-aasland commentedNov 7, 2022•
edited
Loading

@pganssle I can break this up in several PRs; let me know what works best for you.

}

/*[clinic input]
@classmethod
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Do we need thedefining_class converter for@classmethods?

@erlend-aaslanderlend-aasland added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelNov 7, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@erlend-aasland for commit573aec1 🤖

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelNov 7, 2022
Copy link
Member

@sobolevnsobolevn left a comment

Choose a reason for hiding this comment

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

You've beat me to it :)
Thanks!

@erlend-aasland
Copy link
ContributorAuthor

You've beat me to it :)
Thanks!

Sorry, I was not aware that you were working on a PR; I should've asked first.

sobolevn reacted with heart emoji

@erlend-aaslanderlend-aasland added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelNov 15, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@erlend-aasland for commit6e69e93 🤖

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelNov 15, 2022
Copy link
Contributor

@kumaraditya303kumaraditya303 left a comment

Choose a reason for hiding this comment

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

LGTM

@erlend-aasland
Copy link
ContributorAuthor

Thanks for the review, Kumar. I'll wait until Monday with landing this, to give@pganssle a chance to chime in.

@erlend-aaslanderlend-aasland self-assigned thisFeb 8, 2023
Copy link
Member

@pgansslepganssle left a comment

Choose a reason for hiding this comment

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

One minor suggestion here, and it's mostly a documentation thing, otherwise this looks good to me!

Thanks for doing this Erland, sorry for the long delay in reviewing.

Hopefully the tests I put in place for the cache stuff are robust 😅 That kind of thing is hard to test well, and this is the kind of change that really needs it 😛

erlend-aasland reacted with heart emoji
@bedevere-bot
Copy link

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

@erlend-aasland
Copy link
ContributorAuthor

Hopefully the tests I put in place for the cache stuff are robust 😅 That kind of thing is hard to test well, and this is the kind of change that really needs it 😛

Yeah, I know, changes like this need really good test suites! I'll take a look at the coverage before and after the change, just to see that we've at least got all the branches covered.

Thanks for the review 🙂

@erlend-aasland
Copy link
ContributorAuthor

Good to go,@pganssle?

Copy link
Member

@pgansslepganssle left a comment

Choose a reason for hiding this comment

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

Looks good to me! Thanks again for doing this@erlend-aasland and thanks for doing the heavy lifting in the review@kumaraditya303

erlend-aasland reacted with heart emoji
@pganssle
Copy link
Member

I don't think it needs to block this particular PR, but is the subinterpreter work advanced enough that we can add an actual test that the module isolation worked yet?

I'm guessing the answer is no because we need to importdatetime to get the timedeltas, anddatetime is not isolated?

@erlend-aasland
Copy link
ContributorAuthor

I don't think it needs to block this particular PR, but is the subinterpreter work advanced enough that we can add an actual test that the module isolation worked yet?

Subinterpreter testing is not optimal yet. There's some machinery intest_embed /Programs/_testembed.c. See also#98627. Thanks for bringing that up.

I'm guessing the answer is no because we need to importdatetime to get the timedeltas, anddatetime is not isolated?

Correct,_datetime is not isolated yet; I've got a fairly up to date proof-of-concept patch for it, though.

@erlend-aasland
Copy link
ContributorAuthor

Thanks again for the reviews, Kumar and Paul; highly appreciated.

@erlend-aaslanderlend-aasland merged commitc1ce0d1 intopython:mainFeb 15, 2023
@erlend-aaslanderlend-aasland deleted the isolate-zoneinfo branchFebruary 15, 2023 21:58
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment

Reviewers

@sobolevnsobolevnsobolevn approved these changes

@pgansslepgansslepganssle approved these changes

@kumaraditya303kumaraditya303kumaraditya303 approved these changes

Assignees

@erlend-aaslanderlend-aasland

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@erlend-aasland@bedevere-bot@kumaraditya303@pganssle@sobolevn
[8]ページ先頭
©2009-2025 Movatter.jp