◐ Shell
reader mode source ↗
Skip to content

bpo-20182: AC convert remaining functions/methods in _hashopenssl.c#9213

Merged
taleinat merged 5 commits into
python:masterfrom
taleinat:bpo-20182/AC_convert_hashopenssl
Dec 27, 2018
Merged

bpo-20182: AC convert remaining functions/methods in _hashopenssl.c#9213
taleinat merged 5 commits into
python:masterfrom
taleinat:bpo-20182/AC_convert_hashopenssl

Conversation

@taleinat

@taleinat taleinat commented Sep 12, 2018 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

Notes:

  1. This converts four _hashlib.HASH methods, e.g. _hashlib.HASH.copy, despite them not using PyArg_Parse* argument parsing code.
  2. The #include "clinic/_hashopenssl.c.h" line had to be moved down a bit in the file to resolve a compilation issue.

https://bugs.python.org/issue20182

@tiran tiran requested a review from gpshead September 18, 2018 12:48
@taleinat

taleinat commented Sep 18, 2018

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka, thanks for the review! I've addressed your (excellent) comments and would be happy for another review.

@tiran tiran removed the skip news label Sep 18, 2018
@tiran

tiran commented Sep 18, 2018

Copy link
Copy Markdown
Member

I removed the skipnews label. Only trivial PRs are allowed to skip news.

@vstinner, @encukou, @serhiy-storchaka something is wrong with CODEOWNERS for the file. Any change to the hash module should be gated by @gpshead, Alex, or me.

@taleinat

taleinat commented Sep 18, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

something is wrong with CODEOWNERS for the file

How is **/*ssl* not picking up on these files?

Or perhaps @python/crypto-team isn't defined?

@tiran

tiran commented Sep 18, 2018

Copy link
Copy Markdown
Member

It's a mystery. @python/crypto-team exists and I'm a member.

@alex

alex commented Sep 18, 2018

Copy link
Copy Markdown
Member

Can confirm it works well enough that I got an email :D

@serhiy-storchaka

serhiy-storchaka commented Sep 18, 2018

Copy link
Copy Markdown
Member

I think that the NEWS file should contain only information that is interested for end users. Refactorings and conversions to Argument Clinic are not mentioned in it.

gpshead
gpshead approved these changes Sep 21, 2018
View reviewed changes

@gpshead gpshead 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

Thanks for the AC conversions and thanks everyone else for the already great reviews. This looks good.

@taleinat

taleinat commented Sep 22, 2018

Copy link
Copy Markdown
Contributor Author

@tiran, I'd like to re-apply the "skip news" label and merge this. Are you okay with that?

@gpshead

gpshead commented Dec 26, 2018

Copy link
Copy Markdown
Member

I've added the skip news label for you (nothing news worthy about this internal refactoring of _hashopenssl.c). This PR likely needs syncing with merge conflicts addressed before it can go in.

@taleinat

taleinat commented Dec 27, 2018

Copy link
Copy Markdown
Contributor Author

I'd gladly resolve the conflicts and merge this in.

I was waiting for @tiran to approve this; but given that @serhiy-storchaka and @gpshead have approved I guess this is good to go.

@taleinat taleinat merged commit c6c7237 into python:master Dec 27, 2018
@taleinat taleinat deleted the bpo-20182/AC_convert_hashopenssl branch December 27, 2018 13:44
@taleinat taleinat mentioned this pull request Dec 31, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@gpshead gpshead gpshead approved these changes

@encukou encukou encukou approved these changes

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

Assignees

@gpshead gpshead

@tiran tiran

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@taleinat @tiran @alex @serhiy-storchaka @gpshead @encukou @the-knights-who-say-ni @bedevere-bot