Type annotate the ndspy.bmg module
#10
Conversation
RoadrunnerWMC
left a comment
RoadrunnerWMC
left a comment
There was a problem hiding this comment.
Apologies for the delay in looking at this.
This PR seems to include everything from #9 along with the additional annotations, so I'll go ahead and close that in favor of this one.
mike8699
force-pushed
the
finish-bmg-type-annotations
branch
from
d596179 to
8406e7a
Compare
RoadrunnerWMC
left a comment
RoadrunnerWMC
left a comment
There was a problem hiding this comment.
Just a couple more things I noticed after looking over it again, and then I think we'll be good to go!
ndspy/bmg.py
Outdated
|
|
||
| def __init__(self, data=None, *, id=0): | ||
| id: int | ||
| encoding: str |
There was a problem hiding this comment.
Hm, can we make this a Literal too? Valid values for it are listed in the _ENCODINGS constant (ignore the None, that's just a placeholder because encoding values start at 1)
There was a problem hiding this comment.
Done. Sadly it's not possible to pass a list of strings to typing.Literal, so the string literals have to be duplicated between the Literal annotation and the _ENCODINGS list. I believe we could do it the other way around, i.e. something like this
import typing
Encoding = typing.Literal['cp1252', 'utf-16', 'shift-jis', 'utf-8']
_ENCODINGS = typing.get_args(Encoding)But that would break 3.7 compatibility. I don't think it's that big of a deal to have them duplicated, but let me know if you'd prefer it be done a different way. (Plus, if mypy/some other type checker is eventually introduced as a CI check, it would catch any accidental drift between the type annotation and the _ENCODINGS list)
There was a problem hiding this comment.
I think it's good enough to just add a couple comments to indicate that the two should be kept in sync. I can do that after merging the PR.
ndspy/bmg.py
Outdated
| stringParts = None | ||
| isNull = False | ||
| info: bytes | ||
| stringParts: list[str] |
There was a problem hiding this comment.
I almost forgot, this is actually a list[str | Escape], not just of str (this also applies to the corresponding __init__ argument)
mike8699
changed the title
ndspy.bmg modulendspy.bmg module
|
Thanks again! |
2 participants
No description provided.