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.

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

bpo-31862: Port binascii to PEP 489 multiphase initialization #4108

Merged
merged 6 commits into from May 22, 2019

Conversation

ghost
Copy link

@ghost ghost commented Oct 24, 2017

This PR ports _bisect to the PEP 489 multiphase initialization.

The module doesn't use mutable global state, so it should be OK to use with subinterpreters.

This PR is deliberately simple to test the workflow; more and more complicated ones will follow.

https://bugs.python.org/issue31862

@@ -0,0 +1 @@
Port md5 module to PEP 489 multiphase initialization.
Copy link
Member

@merwok merwok Oct 27, 2017

Choose a reason for hiding this comment

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

Do you mean _bisect module?

Copy link
Author

@ghost ghost Oct 30, 2017

Choose a reason for hiding this comment

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

Yes, sorry for that.

@ghost ghost force-pushed the multiphase_PR branch from b03d84f to 0c835cb Compare Oct 30, 2017
@ghost
Copy link
Author

ghost commented Nov 6, 2017

What is the best solution to porting of this many modules?
Should I add all of the modules to this PR, or is it better to merge ported modules in groups in multiple PR's?

@ghost
Copy link
Author

ghost commented Nov 7, 2017

@ncoghlan Would you mind reviewing this?

auvipy
auvipy approved these changes Jan 23, 2018
@ncoghlan
Copy link
Contributor

ncoghlan commented Mar 17, 2018

Hi @Traceur759 - sorry for leaving this unreviewed for so long. Now that @ericsnowcurrently's draft internal API for Python-level subinterpreter support has been added, would you be able to come up with a test case for these modules that fails without the patch, and works with it?

It would also be nice to avoid the module state retrieval overhead in the cases where an exception doesn't need to be raised - I know you have a draft PEP for defining static shared exception types, for this PR specifically you could avoid the problem by deferring loading the state until an exception is about to be raised.

@ghost ghost force-pushed the multiphase_PR branch from 7783481 to 7c2b2cb Compare Mar 19, 2018
@ghost ghost force-pushed the multiphase_PR branch from 11a8a60 to fe696ce Compare Mar 19, 2018
@brettcannon brettcannon changed the title bpo-31862: Port a standard library module to PEP 489 multiphase initialization bpo-31862: Port binascii to PEP 489 multiphase initialization Mar 29, 2019
@the-knights-who-say-ni
Copy link

the-knights-who-say-ni commented Mar 29, 2019

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA (this might be simply due to a missing "GitHub Name" entry in your b.p.o account settings). This is necessary for legal reasons before we can look at your contribution. Please follow the steps outlined in the CPython devguide to rectify this issue.

You can check yourself to see if the CLA has been received.

Thanks again for your contribution, we look forward to reviewing it!

@brettcannon
Copy link
Member

brettcannon commented Mar 29, 2019

Looks like @Dormouse759 doesn't have a bugs.python.org account associated with that GitHub username, so closing as the CLA isn't signed still. This PR can be re-opened if that changes.

@ghost
Copy link
Author

ghost commented Apr 1, 2019

@brettcannon I have renamed my account and forgotten to change the GH name on bugs.python.org.
It should be associated now.

@brettcannon brettcannon reopened this Apr 1, 2019
@brettcannon
Copy link
Member

brettcannon commented Apr 1, 2019

@Dormouse759 thanks for the fix! I've reopened the PEP.

@encukou
Copy link
Member

encukou commented May 7, 2019

@Dormouse759 I rebased on current master, and I have one more suggestion, which is better expressed as a commit rather than text :)
Do you agree with this extra change in binascii_exec?

@ghost
Copy link
Author

ghost commented May 16, 2019

@encukou Yes, it seems fine. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants