8000 MAINT: Remove repeated #ifdefs implementing `isinstance(x, basestring)` for field names by eric-wieser · Pull Request #10418 · numpy/numpy · GitHub
[go: up one dir, main page]
Skip to content

MAINT: Remove repeated #ifdefs implementing isinstance(x, basestring) for field names #10418

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
Merged
merged 1 commit into from
Jan 17, 2018
Merged

MAINT: Remove repeated #ifdefs implementing isinstance(x, basestring) for field names #10418

merged 1 commit into from
Jan 17, 2018

Conversation

eric-wieser
Copy link
Member

cython already defines a __Pyx_PyBaseString_Check for internal use, and googling PyBaseString_Check shows it's not an uncommon name for this macro.

I've based this on the 1.14 branch, just in case

mhvk
mhvk reviewed Jan 17, 2018
View reviewed changes
Copy link
Contributor
@mhvk mhvk left a comment

Choose a reason for hiding this comment

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

Code is OK, but I'm not completely sure about adding something that makes the python3 transition a bit more complicated (since the change then is best undone, as one should try to use the standard python3 calls/macros).

@eric-wieser
Copy link
Member Author

Doesnt this make the py3 transition easier? All we have to do is replace this macro with PyUnicode_Check, which is an easier find and replace than removing ifdefs.

@mhvk
Copy link
Contributor
mhvk commented Jan 17, 2018

Possibly, though right now one mostly has to just search for the ifdefs, but I guess you are right in that we have to do a whole slew of search-and-replace anyway, so adding one more would seem fine...

@eric-wieser
Copy link
Member Author

Restarted the stalled travis job, now passes

@charris charris merged commit 1290e7f into numpy:master Jan 17, 2018
@charris
Copy link
Member
charris commented Jan 17, 2018

Thanks Eric.

@eric-wieser eric-wieser mentioned this pull request Jan 18, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mhvk mhvk mhvk left review comments

Assignees
No one assigned
Labels
03 - Maintenance component: numpy._core
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@eric-wieser @mhvk @charris
0