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.

Sign up for GitHub

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

Jump to bottom

gh-122854: Add Py_HashBuffer() function #122855

Merged
merged 4 commits into from
Aug 30, 2024
Merged

gh-122854: Add Py_HashBuffer() function #122855

merged 4 commits into from
Aug 30, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Aug 9, 2024 •
edited by github-actions bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Aug 9, 2024
Closed
@vstinner
Copy link
Member Author

vstinner commented Aug 9, 2024

cc @pitrou @encukou

erlend-aasland
erlend-aasland reviewed Aug 9, 2024
View reviewed changes
Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

Did you intend to update Misc/stable_abi.toml also?

Doc/c-api/hash.rst Outdated Show resolved Hide resolved
Doc/c-api/hash.rst Outdated Show resolved Hide resolved
encukou
encukou reviewed Aug 12, 2024
View reviewed changes
Copy link
Member

@encukou encukou left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thank you! This looks good, but, wasn't the name supposed to be Py_HashBuffer?

Doc/c-api/hash.rst Outdated Show resolved Hide resolved
picnixz
picnixz reviewed Aug 17, 2024
View reviewed changes
Doc/c-api/hash.rst Outdated Show resolved Hide resolved
Objects/unicodeobject.c Outdated Show resolved Hide resolved
Lib/test/test_capi/test_hash.py Show resolved Hide resolved
@vstinner vstinner changed the title gh-122854: Add Py_HashBytes() function gh-122854: Add Py_HashBuffer() function Aug 26, 2024
@vstinner
Copy link
Member Author

vstinner commented Aug 26, 2024 •
edited
Loading

Thank you! This looks good, but, wasn't the name capi-workgroup/decisions#13 (comment)?

Oh, you're right! I modified the PR to use Py_HashBuffer(). I also rebased the PR to fix a merge conflict.

encukou
encukou reviewed Aug 27, 2024
View reviewed changes
Copy link
Member

@encukou encukou left a comment

Choose a reason for hiding this comment

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

With Erlend's suggestions, this should be good to merge!

Modules/_testcapi/hash.c Outdated Show resolved Hide resolved
@vstinner
Copy link
Member Author

With Erlend's suggestions, this should be good to merge!

Fixed. Oh, I thought that I already addressed his review, but it was maybe on another similar PR.

I also replaced hash_bytes() with hash_buffer() in tests. Nicely spotted @encukou.

@vstinner
Copy link
Member Author

@erlend-aasland:

Did you intend to update Misc/stable_abi.toml also?

capi-workgroup/decisions#13 doesn't mention the stable ABI. Do you think that this API should be added to the stable ABI?

@vstinner vstinner enabled auto-merge (squash) August 30, 2024 15:19
@vstinner
Copy link
Member Author

@erlend-aasland: I enabled auto-merge. If you consider that the function should be added to the limited C API 3.14, I can prepare a follow-up PR.

@vstinner vstinner merged commit d8e69b2 into python:main Aug 30, 2024
37 checks passed
@vstinner vstinner deleted the hashbytes branch August 30, 2024 15:42
@vstinner
Copy link
Member Author

Merged, thanks for reviews.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@erlend-aasland erlend-aasland erlend-aasland left review comments

@picnixz picnixz picnixz left review comments

@encukou encukou encukou left review comments

@pganssle pganssle Awaiting requested review from pganssle pganssle is a code owner

@abalkin abalkin Awaiting requested review from abalkin abalkin is a code owner

@kumaraditya303 kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner

@gpshead gpshead Awaiting requested review from gpshead gpshead is a code owner

@tiran tiran Awaiting requested review from tiran tiran is a code owner

@markshannon markshannon Awaiting requested review from markshannon markshannon is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@vstinner @encukou @picnixz @erlend-aasland