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

Enable py::ellipsis on Python 2 #2360

Merged
merged 2 commits into from
Aug 4, 2020

Conversation

YannickJadoul
Copy link
Collaborator

Not sure why #1502 disabled py::ellipsis for Python 2. ... doesn't seem to exist as a shortcut for Ellipsis (the singleton instance of the ellipsis type; similar to None and NoneType), but Ellipsis still exists (https://docs.python.org/2/library/constants.html#Ellipsis), as well as the C Python interface (https://docs.python.org/2/c-api/slice.html#c.Py_Ellipsis).

Might not be véry useful in Python 2, but I see no reason not to include it, and it broke some tests in #2349 because I forgot to disable that part of the test for Python 2.

Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

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

Looks good to me. I see @ax3l wrote "must be excluded in python 2 tests" in a comment 2 years ago. I'm wondering why, because the Ellipsis object seems to have been there already in Python 2.0.

docs/advanced/pycpp/numpy.rst Outdated Show resolved Hide resolved
@YannickJadoul
Copy link
Collaborator Author

Thanks for the quick review!

I see @ax3l wrote "must be excluded in python 2 tests" in a comment 2 years ago.

I requested a review from @ax3l as well, but my hypothesis is that @wjakob saw that ... didn't work in Python 2 (which it doesn't, but Ellipsis still seems to be working), put preprocessor guards around the py::ellipsis declaration, and @ax3l saw the the tests failing because the test was run on Python 2 as well.

@henryiii
Copy link
Collaborator

henryiii commented Aug 3, 2020

... works everywhere in Python 3, while in Python 2, it only worked inside brackets - but Ellipsis works everywhere in both versions.

@YannickJadoul
Copy link
Collaborator Author

... works everywhere in Python 3, while in Python 2, it only worked inside brackets - but Ellipsis works everywhere in both versions.

Oooooh, nice, OK! That explains a lot. I though I had used it in Python 2 with numpy!

@henryiii
Copy link
Collaborator

henryiii commented Aug 3, 2020

Yes, it was originally added for NumPy, and was special inside slicing, just like : is, but it started being used as a handy token in a few places, so it was enabled everywhere in Python 3. Now it's the recommended choice for Python stubs. :)

@YannickJadoul
Copy link
Collaborator Author

Python 2 on Windows failing seems to have been a fluke?
Also, apparently @ax3l is on holiday, so let's wait for @bstaletic and merge? I don't see how it could break/hurt, but if necessary, we can still revert.

@henryiii
Copy link
Collaborator

henryiii commented Aug 3, 2020

Windows (maybe 2.7 only) iostreams miss the output once in a while. I think we need to investigate further in the future, but not related to this PR.

@YannickJadoul
Copy link
Collaborator Author

Windows (maybe 2.7 only) iostreams miss the output once in a while. I think we need to investigate further in the future, but not related to this PR.

Thanks for the reassurance. Never noticed before, but good to know :-)

@rwgk
Copy link
Collaborator

rwgk commented Aug 3, 2020

Python 2 on Windows failing seems to have been a fluke?
Also, apparently @ax3l is on holiday, so let's wait for @bstaletic and merge? I don't see how it could break/hurt, but if necessary, we can still revert.

Sounds good to me.
General: With @henryiii 's super comprehensive test suite, for super simple fixes like this, I think it's fair to go ahead and merge if we have two approvals, to free up our bandwidth for the many other things we still have piled up.

@YannickJadoul
Copy link
Collaborator Author

Yes, but if @ax3l or @wjakob were not on holiday, I would've been perfectly happy waiting a few days to make sure we didn't miss any reason this was only added for Python 3 :-) (Actually, even now; it's just annoying because #2349 fails; it's perfectly reviewable, though.)

@bstaletic
Copy link
Collaborator

Also, apparently @ax3l is on holiday, so let's wait for @bstaletic and merge? I don't see how it could break/hurt, but if necessary, we can still revert.

Oh, I've seen this already. Looks good to me. Feel free to merge.

@YannickJadoul YannickJadoul merged commit 3e448c0 into pybind:master Aug 4, 2020
@YannickJadoul YannickJadoul deleted the python2-ellipsis branch August 4, 2020 12:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants