Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-121645: Add PyBytes_Join() function#121646
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
Uh oh!
There was an error while loading. Please reload this page.
Conversation
vstinner commented Jul 12, 2024 • edited by github-actions bot
Loading Uh oh!
There was an error while loading. Please reload this page.
edited by github-actions bot
Uh oh!
There was an error while loading. Please reload this page.
vstinner commented Jul 12, 2024
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
vstinner commented Jul 14, 2024
@serhiy-storchaka: Please review the updated PR. I tried to address most of your comments. |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
vstinner commented Jul 17, 2024
@serhiy-storchaka: I addressed most of your comments, except of the one one the doc:
|
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
serhiy-storchaka left a comment
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, but please discuss first the behavior for the NULL separator.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
vstinner commented Jul 24, 2024
@encukou: I updated the PR to address your review. |
cdce8p commented Jul 25, 2024
vstinner commented Jul 27, 2024
vstinner commented Jul 27, 2024
I created capi-workgroup/decisions#36 |
* Replace _PyBytes_Join() with PyBytes_Join(). * Keep _PyBytes_Join() as an alias to PyBytes_Join().
vstinner commented Aug 27, 2024
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 commented Aug 27, 2024
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 commented Aug 27, 2024
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 commented Aug 27, 2024
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 left a comment
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!
I second Serhiy's request to not rebase CPython PRs.
vstinner commented Aug 30, 2024
Merged, thanks for reviews. |
📚 Documentation preview 📚: https://cpython-previews--121646.org.readthedocs.build/