Skip to content

Conversation

@Spider84pr
Copy link
Contributor

Added several missing annotations to serializer.pyi
Also I have added "six" library to Metadata.toml

@github-actions
Copy link
Contributor

github-actions bot commented Nov 8, 2025

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

Copy link
Contributor

@donBarbos donBarbos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! But it seems like adding types-six here is redundant, since only text_type alias for str is used.

version = "1.1.*"
upstream_repository = "https://github.com/html5lib/html5lib-python"

requires = ["six"]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to use types for six, you need to add types-six since the types are distributed separately:

Suggested change
requires = ["six"]
requires = ["types-six"]

Comment on lines +4 to +5
from six import text_type

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See below

Suggested change
from six import text_type

Comment on lines +36 to +37
def encode(self, string: text_type) -> str: ...
def encodeStrict(self, string: text_type) -> str: ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But since you are only using text_type (adding types-six specifically), this doesn't make sense, since text_type is just an alias for str.
Also, both methods could return bytes if self.encoding is set:

Suggested change
def encode(self, string: text_type) -> str: ...
def encodeStrict(self, string: text_type) -> str: ...
def encode(self, string: str) -> str | bytes: ...
def encodeStrict(self, string: str) -> str | bytes: ...

encoding: Incomplete
def serialize(self, treewalker, encoding=None) -> Generator[Incomplete]: ...
def render(self, treewalker, encoding=None): ...
def serialize(self, treewalker: str, encoding: str) -> str: ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Indeed, encoding could accept str, but it is an optional argument (this is what causes problems with CI passing).
  2. treewalker looks like some Iterable of dictionaries (i'd call it _Token), but since we don't have _Token TypedDict yet, I suggest using Iterable[dict[str, Incomplete]]
  3. serialize doesn't just return a string, it yields it, and since with the encoding set, it can be bytes. I think you can use Iterator or Generator
Suggested change
def serialize(self, treewalker: str, encoding: str) -> str: ...
def serialize(self, treewalker: Iterable[dict[str, Incomplete]], encoding: str | None = None) -> Iterator[str | bytes]: ...

def serialize(self, treewalker, encoding=None) -> Generator[Incomplete]: ...
def render(self, treewalker, encoding=None): ...
def serialize(self, treewalker: str, encoding: str) -> str: ...
def render(self, treewalker: str, encoding: str) -> str: ...
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here (except return type)
But I suggest you use overload here to differentiate between cases where str is returned and cases where bytes is returned

Suggested change
def render(self, treewalker: str, encoding: str) -> str: ...
@overload
def render(self, treewalker: Iterable[dict[str, Incomplete]], encoding: Literal[""] | None = None) -> bytes: ...
@overload
def render(self, treewalker: Iterable[dict[str, Incomplete]], encoding: str) -> str: ...

@Spider84pr
Copy link
Contributor Author

@donBarbos Than you very much - i will study all your explanation and follow all tour advices. It will be very useful for me because I am planning to continue contribute to typeshed

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.

2 participants