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-1635741: Port sha256 module to multiphase init (PEP 489) #21189

Merged
merged 4 commits into from
Jul 3, 2020

Conversation

koubaa
Copy link
Contributor

@koubaa koubaa commented Jun 27, 2020

@koubaa koubaa requested a review from tiran as a code owner June 27, 2020 22:42
@the-knights-who-say-ni
Copy link

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept this contribution by verifying everyone involved has signed the PSF contributor agreement (CLA).

Recognized GitHub username

We couldn't find a bugs.python.org (b.p.o) account corresponding to the following GitHub usernames:

@koubaa

This might be simply due to a missing "GitHub Name" entry in one's b.p.o account settings. This is necessary for legal reasons before we can look at this 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 the contribution, we look forward to reviewing it!

@koubaa
Copy link
Contributor Author

koubaa commented Jun 28, 2020

edtited the commit msg and repushed so that it would pick up the CLA which I signed yesterday. Not sure what the Misc/NEWS.d/next is about

Modules/sha256module.c Outdated Show resolved Hide resolved
Modules/sha256module.c Show resolved Hide resolved
Modules/sha256module.c Outdated Show resolved Hide resolved
@shihai1991
Copy link
Member

edtited the commit msg and repushed so that it would pick up the CLA which I signed yesterday. Not sure what the Misc/NEWS.d/next is about

If you add a feature which could effect the user, you should describe what you have changed in Misc/NEWS.d/next.
you can use a web service or cli to generate the description info:

A reference PR of Dong-hee Na:
c4862e3

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

Modules/sha256module.c Outdated Show resolved Hide resolved
Modules/sha256module.c Outdated Show resolved Hide resolved
@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.

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

It's ok to convert static types to heap types in a separated PR.

If @vstinner is okay to make a separate PR for heap types.
I am also okay to merge this PR.

Please apply the review :)

@corona10 corona10 changed the title bpo-1635741: Port sha56 module to multiphase init (PEP 489) bpo-1635741: Port sha256 module to multiphase init (PEP 489) Jun 30, 2020
Modules/sha256module.c Outdated Show resolved Hide resolved
@koubaa
Copy link
Contributor Author

koubaa commented Jul 3, 2020

@corona10 any change still requested?

@shihai1991 @vstinner I started looking at doing heap types. First a disclaimer I'm new in this code so maybe I'm missing some context. I am seeing auto-generated files from clinic that don't take a module pointer as an input, so the module->state approach used by _gdbmmodule would only work if I could modify all of those auto-generated files (and presumably the system which generates them).

Copy link
Member

@vstinner vstinner left a comment

Choose a reason for hiding this comment

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

LGTM.

@corona10, @shihai1991: Would you mind to review the change?

@corona10: Feel free to merge if the change looks good to you ;-)

Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

LGTM Thanks for the work!

@corona10 corona10 merged commit 9d00697 into python:master Jul 3, 2020
@shihai1991
Copy link
Member

oh, sorry, I am late, LGTM.

arun-mani-j pushed a commit to arun-mani-j/cpython that referenced this pull request Jul 21, 2020
@koubaa koubaa deleted the bpo-1635741-sha256 branch August 28, 2020 00:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants