Skip to content

bpo-44231: Remove _PyTuple_FromArray from the public C API #26352

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

Merged
merged 1 commit into from
May 25, 2021

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented May 25, 2021

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented May 25, 2021

I'm not sure if this calls for a NEWS item. Gut feeling says yes :) @pablogsal

@pablogsal
Copy link
Member

I'm not sure if this calls for a NEWS item. Gut feeling says yes :) @pablogsal

It won't hurt, but notice this function is not exposed in the public headers (is in Include/internal) and the removal is not very useful for readers of the changelog as they won't be able to use it.

Copy link
Member

@pablogsal pablogsal 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 would like @vstinner to review this as well, but if he has no time, please ping me in some days so I can land this :)

@erlend-aasland
Copy link
Contributor Author

LGTM

I would like @vstinner to review this as well, but if he has no time, please ping me in some days so I can land this :)

Great, thanks Pablo! :)

@erlend-aasland
Copy link
Contributor Author

I'm not sure if this calls for a NEWS item. Gut feeling says yes :) @pablogsal

It won't hurt, but notice this function is not exposed in the public headers (is in Include/internal) and the removal is not very useful for readers of the changelog as they won't be able to use it.

There was no NEWS entry when it was introduced. That does not imply that we shouldn't include one now, but I believe it's best to keep this strictly internal; a NEWS item may be more confusing than useful in this case.

@vstinner vstinner merged commit 1b940eb into python:main May 25, 2021
@vstinner
Copy link
Member

Merged. I changed the commit message. It's still part of the internal C API, it's just that the symbol is no longer exported.

@vstinner
Copy link
Member

There was no NEWS entry when it was introduced. That does not imply that we shouldn't include one now, but I believe it's best to keep this strictly internal; a NEWS item may be more confusing than useful in this case.

You must not document changes in the internal C API. People must not use it outside CPython. If they use it, they are on their own.

@erlend-aasland
Copy link
Contributor Author

You must not document changes in the internal C API. People must not use it outside CPython. If they use it, they are on their own.

Aye! Thanks for reviewing!

@erlend-aasland erlend-aasland deleted the bpo-44231 branch May 25, 2021 17:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants