-
Notifications
You must be signed in to change notification settings - Fork 33
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
RSDK-6632 - Typescript SDK throws if MotionConfiguration.obstacleDetectorsList is not specified #249
RSDK-6632 - Typescript SDK throws if MotionConfiguration.obstacleDetectorsList is not specified #249
Changes from 3 commits
225b438
34bbc09
f59458c
40a2145
2d3e19c
2502c56
c8dfc90
3a7344b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ export type ObstacleDetector = pb.ObstacleDetector.AsObject; | |
export type LinearConstraint = pb.LinearConstraint.AsObject; | ||
export type OrientationConstraint = pb.OrientationConstraint.AsObject; | ||
export type CollisionSpecification = pb.CollisionSpecification.AsObject; | ||
export type MotionConfiguration = pb.MotionConfiguration.AsObject; | ||
export type MotionConfiguration = Partial<pb.MotionConfiguration.AsObject>; | ||
export type GetPlanResponse = pb.GetPlanResponse.AsObject; | ||
export type ListPlanStatusesResponse = pb.ListPlanStatusesResponse.AsObject; | ||
type ValueOf<T> = T[keyof T]; | ||
|
@@ -71,8 +71,9 @@ export const encodeMotionConfiguration = ( | |
): pb.MotionConfiguration => { | ||
const result = new pb.MotionConfiguration(); | ||
|
||
const { obstacleDetectorsList = [] } = obj; | ||
result.setObstacleDetectorsList( | ||
obj.obstacleDetectorsList.map((od: ObstacleDetector) => { | ||
obstacleDetectorsList.map((od: ObstacleDetector) => { | ||
const obstacleDetector = new pb.ObstacleDetector(); | ||
if (od.visionService) { | ||
obstacleDetector.setVisionService(encodeResourceName(od.visionService)); | ||
|
@@ -83,11 +84,21 @@ export const encodeMotionConfiguration = ( | |
return obstacleDetector; | ||
}) | ||
); | ||
result.setPositionPollingFrequencyHz(obj.positionPollingFrequencyHz); | ||
result.setObstaclePollingFrequencyHz(obj.obstaclePollingFrequencyHz); | ||
result.setPlanDeviationM(obj.planDeviationM); | ||
result.setLinearMPerSec(obj.linearMPerSec); | ||
result.setAngularDegsPerSec(obj.angularDegsPerSec); | ||
if (obj.positionPollingFrequencyHz) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would be safer for these checks to do: if ('positionPollingFrequencyHz' in obj) {
result.setPositionPollingFrequencyHz(obj.positionPollingFrequencyHz)
} The current check will evaluate to false if any of these props are zero, and therefore won't be set, which will cause unexpected behavior. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. with this, when linting, I run into:
I will change the conditionals to be:
this way even if any of the props are zero or false they are set. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That works! |
||
result.setPositionPollingFrequencyHz(obj.positionPollingFrequencyHz); | ||
} | ||
if (obj.obstaclePollingFrequencyHz) { | ||
result.setObstaclePollingFrequencyHz(obj.obstaclePollingFrequencyHz); | ||
} | ||
if (obj.planDeviationM) { | ||
result.setPlanDeviationM(obj.planDeviationM); | ||
} | ||
if (obj.linearMPerSec) { | ||
result.setLinearMPerSec(obj.linearMPerSec); | ||
} | ||
if (obj.angularDegsPerSec) { | ||
result.setAngularDegsPerSec(obj.angularDegsPerSec); | ||
} | ||
|
||
return result; | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How does this behave:
obj
isundefined
?obstacleDetectorsList
is not a member ofobj
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
obj
for this function states that it will never be undefined. If that's not the case, it should be an optional param, and the param should recieve a default value:[]
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Within
typescript-sdk/src/services/motion/client.ts
formoveOnGlobe
andmoveOnMap
we doSo I think it is safe to assume that
obj
will not beundefined
entirely.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case I'd actually recommend making this an optional param like I've done above. If all fields are optional then there's no reason the user should have to pass an empty object into this function.