GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — bpo-1635741: Port sha256 module to multiphase init (PEP 489) by koubaa · Pull Request #21189 · python/cpython

Conversation

@koubaa

@koubaa koubaa commented Jun 27, 2020 •
edited by bedevere-bot
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Link to issue

https://bugs.python.org/issue1635741

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@koubaa
koubaa requested a review from tiran as a code owner June 27, 2020 22:42
@the-knights-who-say-ni

the-knights-who-say-ni commented Jun 27, 2020

Copy link
Copy Markdown

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!
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added the CLA not signed label Jun 27, 2020
@bedevere-bot bedevere-bot added the awaiting review label Jun 27, 2020
@koubaa
koubaa force-pushed the bpo-1635741-sha256 branch from bc4d566 to d63818d Compare June 27, 2020 22:45
@koubaa
Port sha256 module to multi-phase init (PEP 489)
9f9e45a
@koubaa
koubaa force-pushed the bpo-1635741-sha256 branch from d63818d to 9f9e45a Compare June 28, 2020 21:23
@koubaa

koubaa commented Jun 28, 2020

Copy link
Copy Markdown
Contributor Author

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
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@the-knights-who-say-ni the-knights-who-say-ni added CLA signed and removed CLA not signed labels Jun 29, 2020
shihai1991
shihai1991 reviewed Jun 29, 2020
View reviewed changes
Comment thread Modules/sha256module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/sha256module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
Comment thread Modules/sha256module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@shihai1991

shihai1991 commented Jun 29, 2020

Copy link
Copy Markdown
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:

  • https://blurb-it.herokuapp.com/
  • https://pypi.org/project/blurb/

A reference PR of Dong-hee Na:
c4862e3
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner reviewed Jun 30, 2020
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

cc @corona10

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
Comment thread Modules/sha256module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
corona10
corona10 requested changes Jun 30, 2020
View reviewed changes
Comment thread Modules/sha256module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot removed the awaiting review label Jun 30, 2020
@bedevere-bot

bedevere-bot commented Jun 30, 2020

Copy link
Copy Markdown

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.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@bedevere-bot bedevere-bot added the awaiting changes label Jun 30, 2020
corona10
corona10 reviewed Jun 30, 2020
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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 :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@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
koubaa added 2 commits July 1, 2020 20:17
@koubaa
changes requested by review
598fd1a
@koubaa
fix compile issue
5e716b5
vstinner
vstinner reviewed Jul 2, 2020
View reviewed changes
Comment thread Modules/sha256module.c Resolved

Uh oh!

There was an error while loading. Please reload this page.
@koubaa
remove unneeded field inits
300b637
@koubaa

koubaa commented Jul 3, 2020

Copy link
Copy Markdown
Contributor Author

@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).
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
vstinner
vstinner approved these changes Jul 3, 2020
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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 ;-)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@bedevere-bot bedevere-bot added awaiting merge and removed awaiting changes labels Jul 3, 2020
corona10
corona10 approved these changes Jul 3, 2020
View reviewed changes

@corona10 corona10 left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this 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!

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@corona10
corona10 merged commit 9d00697 into python:master Jul 3, 2020
@bedevere-bot bedevere-bot removed the awaiting merge label Jul 3, 2020
@shihai1991

shihai1991 commented Jul 3, 2020

Copy link
Copy Markdown
Member

oh, sorry, I am late, LGTM.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@koubaa
koubaa deleted the bpo-1635741-sha256 branch August 28, 2020 00:21
@kylotan kylotan mannequin mentioned this pull request Sep 19, 2022
Py_Finalize() doesn't clear all Python objects at exit #44470
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shihai1991 shihai1991 shihai1991 left review comments
@vstinner vstinner vstinner approved these changes
@corona10 corona10 corona10 approved these changes
@tiran tiran Awaiting requested review from tiran

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

6 participants

@koubaa @the-knights-who-say-ni @shihai1991 @bedevere-bot @vstinner @corona10
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.