-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Make FFI.buffer actually implement buffer protocol #14505
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
This comment has been minimized.
This comment has been minimized.
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! Looks good, just one remark
stubs/cffi/_cffi_backend.pyi
Outdated
@@ -90,6 +90,7 @@ class _CDataBase: | |||
class buffer: | |||
__hash__: ClassVar[None] # type: ignore[assignment] | |||
def __init__(self, *args, **kwargs) -> None: ... | |||
def __buffer__(self, flags: int) -> memoryview: ... |
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.
flags
is positional-only:
def __buffer__(self, flags: int) -> memoryview: ... | |
def __buffer__(self, flags: int, /) -> memoryview: ... |
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 wasn't sure about that - None of the other definitions in that section have the '/' marking so I left it off for consistency.
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.
Possibly they should too :-) I think these stubs pre-date when /
syntax was available to typeshed. In any case this method doesn't accept keyword arguments, so I think we should reflect that here
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 dunder methods are implicitly treated as positional-only. I might be wrong.
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.
dunder methods are implicitly treated as positional-only
Not exactly. When a C class implements a dunder, it's done through putting a dedicated C function in a C struct (a "slot"). The interpreter then generates a wrapper function that supports calling the dunder directly, and that wrapper function usually (always?) has positional-only arguments. But if define a dunder in Python and call it directly (e.g., something.__buffer__(flags=1)
), it might accept keyword arguments.
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'd be happy to add /
marks for all the dunder methods in this section, but my experience tells me that making something like this inconsistent just wastes the readers mental capacity wondering why this method is marked and this other method is not marked!
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
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!
No description provided.