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

PEP 670: clarify cast; don't change return type #2349

Merged
merged 6 commits into from
Feb 22, 2022
Merged

PEP 670: clarify cast; don't change return type #2349

merged 6 commits into from
Feb 22, 2022

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Feb 21, 2022 •
edited
Loading

  • Clarify how arguments are cast
  • Limited C API 3.11 no longer cast arguments
  • No longer remove return value
  • Require to not change the return type
  • Don't change macros having multiple return types

@vstinner
PEP 670: clarify cast; don't change return type
9a22e13 
* Clarify how arguments are cast
* Limited C API 3.11 no longer cast arguments
* No longer remove return value
* Require to not change the return type
* Don't change macros having multiple return types
@vstinner
Copy link
Member Author

I updated PEP 670 to take in account @encukou and @gpshead feedback on python-dev and on my python/cpython#31221 review.

@encukou @gpshead @erlend-aasland: Would you mind to have a look?

@vstinner
Copy link
Member Author

vstinner commented Feb 21, 2022 •
edited
Loading

You can read the formatted PEP by clicking in the Files Changes tab on the 3 middle dots (...) menu at the pep-0670.rst line: in this menu, click on "View file".

Current link: pep-0670.rst.

CAM-Gerlach
CAM-Gerlach requested changes Feb 21, 2022
View reviewed changes
Copy link
Member

@CAM-Gerlach CAM-Gerlach 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.

Fixes for a number of grammar errors in the added/changed text, as well as a few PEP 12 issues and for correct code block syntax highlighting. All the changes are as suggestions, and can be applied cleanly together by simply going to the Files tab, clicking the Add to batch button on each change, and then clicking the Commit button, which will avoid the all problems we both experienced last time. Alternatively, I'm happy to do it for you. Either way, don't forget to pull your branch after doing so. Thanks!

pep-0670.rst Outdated Show resolved Hide resolved
pep-0670.rst Outdated Show resolved Hide resolved
pep-0670.rst Outdated Show resolved Hide resolved
pep-0670.rst Outdated Show resolved Hide resolved
pep-0670.rst Outdated Show resolved Hide resolved
pep-0670.rst Outdated Show resolved Hide resolved
pep-0670.rst
* ``_Py_atomic_store_64bit()``
* ``asdl_seq_SET()``
* ``asdl_seq_SET_UNTYPED()``
For example, Python 3.7 changed ``PyUnicode_AsUTF8()`` return type from
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
For example, Python 3.7 changed ``PyUnicode_AsUTF8()`` return type from
For example, Python 3.7 changed ``PyUnicode_AsUTF8()``\ 's return type from

Fix grammar error

Copy link
Member

Choose a reason for hiding this comment

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

This grammar error wasn't fixed

pep-0670.rst Outdated Show resolved Hide resolved
pep-0670.rst Outdated Show resolved Hide resolved
pep-0670.rst Outdated Show resolved Hide resolved
erlend-aasland
erlend-aasland approved these changes Feb 22, 2022
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.

I'm fine with this, Victor.

@vstinner
Copy link
Member Author

@CAM-Gerlach: I addressed most of your reviews, thanks.

@vstinner vstinner merged commit 42e921e into python:main Feb 22, 2022
@vstinner vstinner deleted the pep670_cast branch February 22, 2022 12:17
@vstinner
Copy link
Member Author

Follow-up: I wrote an email to python-dev: https://mail.python.org/archives/list/python-dev@python.org/thread/VM6I3UHVMME6QRSUOYLK6N2OZHP454W6/

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commented Feb 22, 2022 •
edited by AA-Turner
Loading

Good call on using the language:: c directive in lieu of .. code-block:: c on all the code blocks, but it looks like at least a few of the grammar errors cited were not fixed (FYI, you can edit them, e.g. to remove the code-block directive, and apply those instead, to avoid them getting missed in the future). If I do get the chance to do a full copyedit of the PEP (I've been busy with other PEP editor stuff, sorry), I'll clean those up.

@erlend-aasland erlend-aasland mentioned this pull request May 13, 2022
Closed
@erlend-aasland erlend-aasland mentioned this pull request Jun 27, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CAM-Gerlach CAM-Gerlach CAM-Gerlach requested changes

@erlend-aasland erlend-aasland erlend-aasland approved these changes

@encukou encukou Awaiting requested review from encukou

@gpshead gpshead Awaiting requested review from gpshead

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

Successfully merging this pull request may close these issues.
4 participants
@vstinner @CAM-Gerlach @erlend-aasland @the-knights-who-say-ni