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-121645: Add PyBytes_Join() function #121646

Merged
merged 2 commits into from
Aug 30, 2024
Merged

gh-121645: Add PyBytes_Join() function #121646

merged 2 commits into from
Aug 30, 2024

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Jul 12, 2024 •
edited by github-actions bot
Loading

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

cc @erlend-aasland @serhiy-storchaka

erlend-aasland
erlend-aasland reviewed Jul 12, 2024
View reviewed changes
Doc/c-api/bytes.rst Outdated Show resolved Hide resolved
serhiy-storchaka
serhiy-storchaka reviewed Jul 12, 2024
View reviewed changes
Doc/c-api/bytes.rst Outdated Show resolved Hide resolved
Doc/whatsnew/3.14.rst Outdated Show resolved Hide resolved
Lib/test/test_capi/test_bytes.py Show resolved Hide resolved
picnixz
picnixz reviewed Jul 12, 2024
View reviewed changes
Objects/bytesobject.c Outdated Show resolved Hide resolved
Doc/whatsnew/3.14.rst Outdated Show resolved Hide resolved
@vstinner
Copy link
Member Author

@serhiy-storchaka: Please review the updated PR. I tried to address most of your comments.

@cdce8p cdce8p mentioned this pull request Jul 14, 2024
Closed
serhiy-storchaka
serhiy-storchaka reviewed Jul 15, 2024
View reviewed changes
Doc/c-api/bytes.rst Outdated Show resolved Hide resolved
Lib/test/test_capi/test_bytes.py Outdated Show resolved Hide resolved
Include/cpython/bytesobject.h Outdated Show resolved Hide resolved
Modules/_io/bufferedio.c Show resolved Hide resolved
Lib/test/test_capi/test_bytes.py Outdated Show resolved Hide resolved
Objects/bytesobject.c Outdated Show resolved Hide resolved
@vstinner
Copy link
Member Author

@serhiy-storchaka: I addressed most of your comments, except of the one one the doc:

NULL which is treated as an empty string.

serhiy-storchaka
serhiy-storchaka reviewed Jul 18, 2024
View reviewed changes
Objects/bytesobject.c Outdated Show resolved Hide resolved
Doc/c-api/bytes.rst Outdated Show resolved Hide resolved
serhiy-storchaka
serhiy-storchaka approved these changes Jul 18, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM, but please discuss first the behavior for the NULL separator.

@cdce8p cdce8p mentioned this pull request Jul 21, 2024
Closed
@vstinner vstinner mentioned this pull request Jul 23, 2024
Closed
encukou
encukou reviewed Jul 24, 2024
View reviewed changes
Lib/test/test_capi/test_bytes.py Show resolved Hide resolved
Doc/whatsnew/3.14.rst Outdated Show resolved Hide resolved
Doc/c-api/bytes.rst Outdated Show resolved Hide resolved
@vstinner
Copy link
Member Author

@encukou: I updated the PR to address your review.

This was referenced Jul 25, 2024
Closed
Merged
@cdce8p
Copy link
Contributor

cdce8p commented Jul 25, 2024

@vstinner This might need a merge / rebase after #122267.

@vstinner
Copy link
Member Author

@vstinner This might need a merge / rebase after #122267.

Done.

@vstinner
Copy link
Member Author

I created capi-workgroup/decisions#36

@vstinner
pythongh-121645: Add PyBytes_Join() function
6353240 
* Replace _PyBytes_Join() with PyBytes_Join().
* Keep _PyBytes_Join() as an alias to PyBytes_Join().
@vstinner
Copy link
Member Author

I created capi-workgroup/decisions#36

It was decided to reject NULL separator. I updated my PR to respect the C API Working Group decision.

I also rebased the PR to fix merge conflicts.

@erlend-aasland @encukou @serhiy-storchaka: Would you mind to review the updated PR?

@serhiy-storchaka
Copy link
Member

Please do not use rebase so later in the review. So I do not see difference with already reviewed code and need to re-read all from the start.

Last two days I have power only for 2 intervals of 2 hours per day, so new review may take a time.

@vstinner
Copy link
Member Author

Please do not use rebase so later in the review.

Do you mean that rebasing on main is bad, or is squashing commits which is bad for review? Sorry, I will avoid squashing commits next time.

@serhiy-storchaka
Copy link
Member

Normally, I can find a link "changes since your last review". It is the best scenario.

If you rebased without squashing, I can still manually select recent commits. This is less convenient, and there is no guarantee that you did not modify previous commits.

If you squashed commits, all is lost. I do this only immediately after creating commit or pushing new changes, when there is a little chance that my previous change has been reviewed.

encukou
encukou approved these changes Aug 29, 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.

LGTM!

I second Serhiy's request to not rebase CPython PRs.

@vstinner vstinner enabled auto-merge (squash) August 30, 2024 12:32
@vstinner vstinner merged commit 3d60dfb into python:main Aug 30, 2024
37 checks passed
@vstinner vstinner deleted the bytes_join branch August 30, 2024 12:57
@vstinner
Copy link
Member Author

Merged, thanks for reviews.

@hugovk hugovk mentioned this pull request Sep 12, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cdce8p cdce8p cdce8p left review comments

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

@picnixz picnixz picnixz left review comments

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

@encukou encukou encukou approved these changes

Assignees
No one assigned
Labels
topic-C-API
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vstinner @cdce8p @serhiy-storchaka @encukou @picnixz @erlend-aasland