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

guide: add hints for the signature of pymethods protos #1964

Merged
merged 1 commit into from Nov 3, 2021

Conversation

birkenfeld
Copy link
Member

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.

Copy link
Member

@davidhewitt davidhewitt left a 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

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.
Copy link
Member

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.

Copy link
Member Author

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.

`__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
Copy link
Member

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".

Suggested change
- 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
Suggested change
- 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

- `__delattr__`
- `__bool__`
- `__call__`
- `__str__() -> object (str)`
Copy link
Member

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?

Suggested change
- `__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!

Copy link
Member Author

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>.

Copy link
Member

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.

@birkenfeld
Copy link
Member Author

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.

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.

@birkenfeld
Copy link
Member Author

Updated!

Copy link
Member

@davidhewitt davidhewitt left a 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!

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.

None yet

2 participants