Skip to content

Conversation

jckras
Copy link
Member

@jckras jckras commented Jun 2, 2023

Follow-up work to be done: https://viam.atlassian.net/browse/RSDK-3403

@jckras jckras requested review from stuqdog and cheukt June 2, 2023 14:18
@jckras jckras requested a review from a team as a code owner June 2, 2023 14:18
@CLAassistant

This comment was marked as resolved.

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.

A couple small suggestions, but otherwise this looks great! thanks for putting it together :)

return {"command": command}



Copy link
Member

Choose a reason for hiding this comment

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

(nit) probably we should get rid of this newline

ResourceName(namespace="rdk", type="component", subtype="arm", name="arm1"),
ResourceName(namespace="rdk", type="component", subtype="camera", name="camera1"),
ResourceName(namespace="rdk", type="component", subtype="motor", name="motor1"),
# ResourceName(namespace="rdk", type="component", subtype="sensor", name="movement_sensor1"),
Copy link
Member

Choose a reason for hiding this comment

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

As a general policy, I think it's best to not have commented code committed. It is often unclear what purpose it serves/used to serve, and there's no validation that the code still works as expected. Probably best to get rid of this entirely.

@jckras jckras requested a review from stuqdog June 5, 2023 15:52
@jckras
Copy link
Member Author

jckras commented Jun 5, 2023

Re-requested review after updating the tests to account for the dedupe done in generate metadata. Also added dedupe in generateStatus

Copy link
Member

@cheukt cheukt left a comment

Choose a reason for hiding this comment

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

Looks great! small nit

client = RobotServiceStub(channel)
response: GetStatusResponse = await client.GetStatus(GetStatusRequest())
assert list(response.status) == STATUSES
assert list(response.status) == (STATUSES)
Copy link
Member

@cheukt cheukt Jun 5, 2023

Choose a reason for hiding this comment

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

nit: extra parenthesis?

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.

LGTM!

@jckras jckras merged commit e4923a9 into viamrobotics:main Jun 5, 2023
@jckras jckras deleted the fix4sensors branch June 5, 2023 20:01
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.

4 participants