-
Notifications
You must be signed in to change notification settings - Fork 234
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
refactor: add type hints to protocol #997
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.
Hi @odysa, thank you for your effort to contribute. Unfortunately there is a problem in original code that concrete classes don't match the base class. And we definitely have to solve it before merging.
@classmethod | ||
@abc.abstractmethod | ||
def encode(cls, value): ... | ||
def encode(self, value: Optional[T]) -> bytes: ... |
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.
Why self
if it's declared as classmethod
?
import struct | ||
from struct import error | ||
from typing import Callable, Optional, TypeVar | ||
|
||
from _typeshed import ReadableBuffer |
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.
It's not a public module
def __init__(self, *fields): | ||
if fields: | ||
self.names, self.fields = zip(*fields) | ||
else: | ||
self.names, self.fields = (), () | ||
|
||
def encode(self, item): | ||
def encode(self, item) -> bytes: |
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.
Here is the problem: it violates the declaration in the abstract base class. And I see no simple way to fix it.
This PR seem to have been superseded, should it be closed? |
Changes
Add type hints to the protocol module
Checklist
CHANGES
folder<issue_id>.<type>
(e.g.588.bugfix
)issue_id
change it to the pr id after creating the PR.feature
: Signifying a new feature..bugfix
: Signifying a bug fix..doc
: Signifying a documentation improvement..removal
: Signifying a deprecation or removal of public API..misc
: A ticket has been closed, but it is not of interest to users.Fix issue with non-ascii contents in doctest text files.