add Ellipsis() and is_ellipsis() methods#2911
Conversation
samuelcolvin
force-pushed
the
ellipsis
branch
from
3536f36 to
d097638
Compare
|
Seems to really be failing on windows, error is Any ideas? |
I'm guessing it needs a |
Ha, I'm trying exactly that having seen it here. |
|
Would be great to get this merged and released as I'm currently having to get pyo3 from my branch due to the windows issue. 🚀 🏃 |
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Sorry for the delay. My family's sick with a cold again....
I think there's some API details which I'd like to at least discuss whether we can do better before we merge.
| /// Gets the Python builtin value `Ellipsis`, or `...`. | ||
| #[allow(non_snake_case)] // the Python keyword starts with uppercase | ||
| #[inline] | ||
| pub fn Ellipsis(self) -> PyObject { |
There was a problem hiding this comment.
Rather than returning PyObject here it would probably be nicer if we returned &PyEllipsis, and created a new struct types::PyEllipsis. (Would avoid the need to increase a reference count here.)
That would also mean that x.is_instance::<PyEllipsis>() would just work instead of needing is_ellipsis, which does have the upside of avoiding a special method. Not necessarily saying that it's better, perhaps is_ellipsis is still warranted.
Also, adding types::PyEllipsis would pave the way for instead just having PyEllipsis::get(py) to retrieve the singleton, rather than adding this to Python which I don't think really needs this method.
There was a problem hiding this comment.
Then we should do this for None and NotImplemented too. The current pr mirrors their api.
There was a problem hiding this comment.
Agreed, and I've been wanting to do that for a while. I guess we can't change those for 0.18.x, so perhaps we merge this as-is and I'll probably raise a follow-up PR to propose adjusting all three on the way to 0.19?
davidhewitt
left a comment
davidhewitt
left a comment
There was a problem hiding this comment.
Approved as per comment that I'll try to find some time to add to this on the way to 0.19.
I've just force-pushed to squash-and-rebase, will merge. Thanks!
|
bors r=mejrs,davidhewitt |
|
Build succeeded: |
3 participants
fix #2906
Please consider adding the following to your pull request: