-
Notifications
You must be signed in to change notification settings - Fork 826
guide: add hints for the signature of pymethods protos #1964
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
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.
Thanks for kicking this off!
I keep wondering... Should we add variable names too? name: ty
is common to both rust and Python, and it'll help users know what to do with the incoming arguments.
By the way, I discovered that it is possible to omit arguments to the methods, e.g. to define a
__richcmp__
without arguments. It seems to be handled all right, just not passing the slot arguments to Rust, but I wonder if that's intentional.
Yeah I kinda knew about this and was thinking it was a minor bug. It probably should be checked and a nice error message emitted. Users missing arguments off is probably an incorrect implementation imo so we should at least force them to be explicit about unused arguments.
The code responsible is
pyo3/pyo3-macros-backend/src/pymethod.rs
Line 955 in 0f92f28
fn extract_proto_arguments( |
method_args
are the arguments the user wrote for the method; at the moment the code just iterates it without checking its length. Slightly complicated by the Python
arguments but not too bad.
- All methods take a receiver as first argument, which can be `&self`, `&mut | ||
self` or a `PyCell` reference like `self_: PyRef<Self>` and `self_: | ||
PyRefMut<Self>`, as described [here](../class.md#inheritance). | ||
- An optional `Python<'py>` argument is always allowed as the first argument. |
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.
I think in the way currently implemented Python
arguments can be in any position. I'd be happy to restrict this, however.
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.
ah ok, I didn't even test this. I suggest keeping it like this in the docs anyway.
guide/src/class/protocols.md
Outdated
`__richcmp__`'s second argument. | ||
- For the comparison and arithmetic methods, extraction errors are not | ||
propagated as exceptions, but lead to a return of `NotImplemented`. | ||
- For some slots, the return values are not restricted by PyO3, but checked by |
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.
I've been trying to stick to "magic method".
- For some slots, the return values are not restricted by PyO3, but checked by | |
- For some slots, the return values are not restricted by PyO3, but checked by |
- For some slots, the return values are not restricted by PyO3, but checked by | |
- For some magic methods, the return values are not restricted by PyO3, but checked by |
guide/src/class/protocols.md
Outdated
- `__delattr__` | ||
- `__bool__` | ||
- `__call__` | ||
- `__str__() -> object (str)` |
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.
Although arguably redundant, should receivers be included?
- `__str__() -> object (str)` | |
- `__str__(&self) -> object (str)` |
I guess we don't want to pin users to a single receiver type, so seeing this here will either remind them to include it or confuse them!
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.
Or use something like <self>
.
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.
<self>
sounds like a great idea to me; seems like a good placeholder that's clearly not valid syntax.
As long as we don't also add a blurb about what the function does, this might not make it much clearer. When the old system is deprecated, or when the new system is finalized, I'd like to move the descriptions from the old chapters below to the new chapter; maybe at this point it would make sense to add names. On the other hand, the short form makes it clear that these are not copy-pastable signatures. |
b69c241
to
db8753f
Compare
db8753f
to
9ce363a
Compare
Updated! |
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.
Thanks, I'm glad we have this in time for the 0.15 release!
A start to give a hint about pymethod-proto args.
By the way, I discovered that it is possible to omit arguments to the methods, e.g. to define a
__richcmp__
without arguments. It seems to be handled all right, just not passing the slot arguments to Rust, but I wonder if that's intentional.