-
-
Notifications
You must be signed in to change notification settings - Fork 18.6k
CLN: Update cpython imports in Cython Code GH28382 #28398
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
Conversation
from cpython.long cimport PyLong_Check | ||
from cpython.object cimport PyCallable_Check | ||
from cpython.tuple cimport PyTuple_Check | ||
from cpython.unicode cimport PyUnicode_Check, PyUnicode_AsEncodedString |
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.
doesnt need to be for this PR, but most/all of the PyFoo_Check calls can be replaced with their pure-python equivalents; cython will now automatically make the optimization
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.
So basically replace this with isinstance
checks right? I think OK as a separate PR @amunch if that's something you'd also like to tackle
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.
Sounds good, I will replace these checks in a separate pull request.
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.
Looks good. Can you merge master and repush? I think will resolve CI failure
from cpython.long cimport PyLong_Check | ||
from cpython.object cimport PyCallable_Check | ||
from cpython.tuple cimport PyTuple_Check | ||
from cpython.unicode cimport PyUnicode_Check, PyUnicode_AsEncodedString |
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.
So basically replace this with isinstance
checks right? I think OK as a separate PR @amunch if that's something you'd also like to tackle
@WillAyd Looks like merging master resolved the CI issues! |
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 @jbrockmendel
LGTM. Should we add a code check for this? |
Not against it if someone wants to take that up, but don't think needs to be done as part of this PR |
Thanks @amunch - great first PR |
black pandas
git diff upstream/master -u -- "*.py" | flake8 --diff
Apologies if anything is not ideal, as this is my first pull request here. Any suggestions or comments are much appreciated!