Skip to content

Conversation

njooma
Copy link
Member

@njooma njooma commented Nov 15, 2023

No description provided.

@njooma njooma requested a review from a team as a code owner November 15, 2023 21:14
Copy link
Member

@benjirewis benjirewis left a comment

Choose a reason for hiding this comment

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

@purplenicole730 probably has more context than I do, but seems good!

@njooma njooma requested a review from stuqdog November 15, 2023 23:58
@njooma
Copy link
Member Author

njooma commented Nov 15, 2023

@purplenicole730 is on vacay, so pinging @stuqdog instead

response = await stream.recv_message()
if not response:
await stream.recv_trailing_metadata() # causes us to throw appropriate gRPC error
raise TypeError("Response cannot be empty")
Copy link
Member

Choose a reason for hiding this comment

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

(q) Is it possible for us to actually reach this point? if not response is the case when the response isn't just empty, but a None, which I think should only happen if something went wrong such that we'd have a non-OK gRPC status and so throw an error at the preceding line. I guess either way there's no harm in having another safety rail here!

Copy link
Member Author

@njooma njooma Nov 16, 2023

Choose a reason for hiding this comment

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

WE know we can't get here, but the typechecker doesn't know that and it was complaining

Copy link
Member

Choose a reason for hiding this comment

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

Ahh got it, thanks!

Copy link
Member

@stuqdog stuqdog left a comment

Choose a reason for hiding this comment

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

one question, but looks good to me!

@njooma njooma merged commit d2cb35e into viamrobotics:main Nov 16, 2023
@njooma njooma deleted the DATA-2065/file-upload branch November 16, 2023 17:41
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