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

refactor: add type hints to protocol #997

Closed
wants to merge 2 commits into from

Conversation

odysa
Copy link

@odysa odysa commented Apr 9, 2024

Changes

Add type hints to the protocol module

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • Documentation reflects the changes
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> (e.g. 588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the PR
    • ensure type is one of the following:
      • .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.
    • Make sure to use full sentences with correct case and punctuation, for example: Fix issue with non-ascii contents in doctest text files.

Copy link
Collaborator

@ods ods left a 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.

Comment on lines 12 to +14
@classmethod
@abc.abstractmethod
def encode(cls, value): ...
def encode(self, value: Optional[T]) -> bytes: ...
Copy link
Collaborator

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
Copy link
Collaborator

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:
Copy link
Collaborator

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.

@antonagestam
Copy link

This PR seem to have been superseded, should it be closed?

@ods
Copy link
Collaborator

ods commented Jul 6, 2024

Already implemented in #999 and #1005

@ods ods closed this Jul 6, 2024
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.

3 participants