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.

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

add missing unicode APIs #4910

Merged
merged 9 commits into from
Jul 26, 2022
Merged

add missing unicode APIs #4910

merged 9 commits into from
Jul 26, 2022

Conversation

maxbachmann
Copy link
Contributor

Most other functions in this CPython API files come with the Python documentation (e.g. the C++ API does not come with docs). They tend to get easily out of sync with the official CPython documentation and can get quite long. However if you prefer to have the documentation added I can copy them over from the Cpython docs.

@scoder scoder added this to the 3.0 milestone Jul 21, 2022
@scoder
Copy link
Contributor

scoder commented Jul 21, 2022

Most other functions in this CPython API files come with the Python documentation (e.g. the C++ API does not come with docs). They tend to get easily out of sync with the official CPython documentation and can get quite long.

I agree about this getting outdated. Sending users to the official documentation seems generally better. I think we should reserve comments to anything specific that we have to add from Cython side (which is rare), and leave out any texts that are maintained elsewhere.

@maxbachmann
Copy link
Contributor Author

I agree about this getting outdated. Sending users to the official documentation seems generally better. I think we should reserve comments to anything specific that we have to add from Cython side (which is rare), and leave out any texts that are maintained elsewhere.

should the unneeded parts be removed -> just leave Cython specific information and information when API was added / will be removed?

@scoder
Copy link
Contributor

scoder commented Jul 22, 2022

I think we should reserve comments to anything specific that we have to add from Cython side (which is rare), and leave out any texts that are maintained elsewhere.

should the unneeded parts be removed -> just leave Cython specific information and information when API was added / will be removed?

Not in this PR, but yes, I think we can start removing the old doc copy comments.

Cython/Includes/cpython/unicode.pxd Outdated Show resolved Hide resolved
@@ -1,4 +1,9 @@
from libc.stdint cimport uint8_t, uint16_t
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't remember – is <stdint.h> safe to include in a platform independent way these days?

Copy link
Contributor Author

@maxbachmann maxbachmann Jul 22, 2022

Choose a reason for hiding this comment

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

Hm apparently this is a c99/c++89 feature and only available in msvc since the 2010 version. It appears we already use it in https://github.com/cython/cython/blob/master/Cython/Includes/cpython/time.pxd which might need to be fixed as well. I replaced it with

ctypedef unsigned char  uint8_t
ctypedef unsigned short uint16_t

for now.

maxbachmann and others added 2 commits July 22, 2022 14:00
Co-authored-by: scoder <stefan_ml@behnel.de>
@maxbachmann
Copy link
Contributor Author

Not in this PR, but yes, I think we can start removing the old doc copy comments.

yes I will open a different PR on this

Cython/Includes/cpython/unicode.pxd Outdated Show resolved Hide resolved
Cython/Includes/cpython/unicode.pxd Outdated Show resolved Hide resolved
maxbachmann and others added 2 commits July 24, 2022 19:20
Co-authored-by: scoder <stefan_ml@behnel.de>
Co-authored-by: scoder <stefan_ml@behnel.de>
@scoder scoder merged commit ea38521 into cython:master Jul 26, 2022
@scoder
Copy link
Contributor

scoder commented Jul 26, 2022

Thanks

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.

None yet

3 participants