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

Use protoc's --pyi_out instead of mypy-protobuf package #174

Merged
merged 30 commits into from
Mar 20, 2024
Merged

Conversation

keepingitneil
Copy link
Contributor

@keepingitneil keepingitneil commented Mar 19, 2024

And add type checking GHA

Successful type checking run: https://github.com/livekit/python-sdks/actions/runs/8351262508

Purposely failed type checking run by adding bad_variable: str = 5 to a few files: https://github.com/livekit/python-sdks/actions/runs/8351236763

@keepingitneil keepingitneil marked this pull request as ready for review March 19, 2024 23:07
@@ -1,4 +1,4 @@
# Copyright 2023 LiveKit, Inc.
# Copyright 2024 LiveKit, Inc.
Copy link
Member

Choose a reason for hiding this comment

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

don't need to change this

@theomonnom
Copy link
Member

Is changing mypy to pyi generator a breaking change?

@@ -80,9 +81,9 @@ class RoomOptions:
class DataPacket:
data: bytes
kind: proto_room.DataPacketKind.ValueType
Copy link
Member

@theomonnom theomonnom Mar 20, 2024

Choose a reason for hiding this comment

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

Weird thing is that ValueType doesn't exist anymore due to the pyi generator but mypy is not complaining ? Seems like the type checking isn't working? Is the code running ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh this is Room is not from livekit-protocol, it's from livekit/rtc/_proto which is still generated by mypy-protobuf.

Copy link
Contributor Author

@keepingitneil keepingitneil Mar 20, 2024

Choose a reason for hiding this comment

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

Just to double check, changed kind to something bad (i.e. Dict[str, str]) and ran it again and it failed as expected: https://github.com/livekit/python-sdks/actions/runs/8362789419

@theomonnom
Copy link
Member

theomonnom commented Mar 20, 2024

The mypy generator may be a better choice for stricter type checking if the types are not detected (the above comment)

@keepingitneil
Copy link
Contributor Author

The mypy generator may be a better choice for stricter type checking if the types are not detected (the above comment)

Yeah in my testing pyi_out has been working flawlessly. Since it's newer, a bit more "official", and removes a build dependency I decided to make the switch. I left livekit-rtc in-tact but we can switch to pyi_out for those protos too in another PR if you're satisfied with the pyi_out typing.

@keepingitneil
Copy link
Contributor Author

Is changing mypy to pyi generator a breaking change?

It should not be. It only affects the pyi files. The generated py files remain as-is.

Copy link
Member

@theomonnom theomonnom left a comment

Choose a reason for hiding this comment

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

nice!

@keepingitneil keepingitneil merged commit 574fe4e into main Mar 20, 2024
18 checks passed
@keepingitneil keepingitneil deleted the neil/mypy branch March 20, 2024 17:47
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