[RSDK-878] Typechecking #512
Conversation
|
Warning your change may break code samples. If your change modifies any of the following functions please contact @viamrobotics/fleet-management. Thanks!
|
|
@randhid @penguinland FYI this PR removes the subclassing from GPIOPin, AnalogReader, DigitalIterrupt |
| with open(dest, "wb") as file: | ||
| file.write(data) | ||
| stream: Stream[GetInvoicePdfRequest, GetInvoicePdfResponse] | ||
| async with self._billing_client.GetInvoicePdf.open(timeout=timeout, metadata=self._metadata) as stream: |
There was a problem hiding this comment.
type checking surfaced this issue, where we weren't streaming the response properly
| myst-nb = [ | ||
| {version = "<1.0.0", python = "<3.9"}, | ||
| {version = ">=1.0.0", python = ">=3.9"}, | ||
| ] | ||
| sphinx-autoapi = [ | ||
| {version = "<3.0.0", python = "<3.9"}, | ||
| {version = ">=3.0.0", python = ">=3.9"}, | ||
| ] |
There was a problem hiding this comment.
Updating deps only support >3.9
| @@ -44,6 +45,8 @@ def getLogger(name: str) -> logging.Logger: | |||
|
|
|||
|
|
|||
| def addHandlers(logger: logging.Logger): | |||
| logger.handlers.clear() | |||
There was a problem hiding this comment.
This is to fix an issue where logs were getting doubled if the level was changed
| @@ -41,15 +41,15 @@ async def get_position(self, *, timeout: Optional[float] = None) -> Pose: | |||
| response: GetPositionResponse = await self.client.GetPosition(request, timeout=timeout) | |||
| return response.pose | |||
|
|
|||
| async def get_point_cloud_map(self, *, timeout: Optional[float] = None) -> List[GetPointCloudMapResponse]: | |||
| async def get_point_cloud_map(self, *, timeout: Optional[float] = None) -> List[bytes]: | |||
There was a problem hiding this comment.
type checking surfaced these incorrect return types
src/viam/app/app_client.py
Outdated
| return response.url | ||
| await stream.recv_trailing_metadata() # causes us to throw appropriate gRPC error. |
There was a problem hiding this comment.
It looks like this got inverted from how it was before? we want to recv_trailing_metadata() to throw an error, when response is None.
There was a problem hiding this comment.
Not sure why I changed it, but isn't this functionally the same?
There was a problem hiding this comment.
🤦 yes you're absolutely right, read it too quickly and thought the if statements were equivalent.
I do have a slight preference for either leaving it the way it was or updating the equivalent logic in data_client.py and audo_input/client.py just to all be consistent, but I don't feel strongly enough to hold off release.
Add typechecking to the python SDK. I chose pyright over mypy for a few reasons:
We will likely still want to eventually move over to mypy, but this is a great starting place for us.
Some notes on why I did what I did:
__eq__functions__eq__methods that were typed to the type of self, it was an incorrect overrideSUBTYPEin resourcesSUBTYPEfield is required for all resource types (anything that extends fromResourceBase). When we actually implement that in our concrete subtypes, we want it to beFinal. However, since the original abstract definition is notFinal(because it's abstract), the typechecker views it as an incompatible override. So I ignore it**__in method functions**kwargsas parameters, those need to be included in the client to make a valid override. However, those would go unused. So I added**__to signify unused kwargsself.get_resource, it's correctly typedComponentBasefrom_robot