Refactor: Exhaustive Bridge Protocol #1072
Conversation
…tion based on the Rosbridge Protocol Signed-off-by: Drew Hoener <[email protected]>
Signed-off-by: Drew Hoener <[email protected]>
Signed-off-by: Drew Hoener <[email protected]>
…d on "op" field instead of all the checker functions. Enforce typing of messages and callback types. Fix deviations from protocol spec Signed-off-by: Drew Hoener <[email protected]>
… objects. Add helper types to increase readability in constructors and function parameters. Signed-off-by: Drew Hoener <[email protected]>
…ould have. Fixes RobotWebTools#1068 Signed-off-by: Drew Hoener <[email protected]>
…d check works Signed-off-by: Drew Hoener <[email protected]>
…lomatic Complexity and increase readability. Signed-off-by: Drew Hoener <[email protected]>
…ssary. Signed-off-by: Drew Hoener <[email protected]>
EzraBrooks
left a comment
EzraBrooks
left a comment
There was a problem hiding this comment.
Could you break this down into more atomic PRs? I'm finding it hard to determine which changes are related to which.
| * @param obj The object to check. | ||
| * @param prop The property to check for. | ||
| */ | ||
| export function hasOwn<X extends object, Y extends PropertyKey>( |
There was a problem hiding this comment.
Did you know the in operator already does this type guard?
There was a problem hiding this comment.
in will also check the prototype chain which isn't always something you want. Though for the places I use it I suppose I could just use in
There was a problem hiding this comment.
Yep - however, given that we have no inheritance other than from Object on these messages, I think it's fine
| RosActionEvents & // Action Events | ||
| RosServiceEvents & | ||
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| Record<TopicKey, [message: any]>; // Service Calls & Topic messages, need to be `any` to be coerced by implementers |
There was a problem hiding this comment.
Please use unknown instead of any.
|
I had a brain blast on how to (tediously, but thoroughly) strictly enforce the message types everywhere. I'll put up a PR for your review |
|
please let me know what you think about #1075 - it caught the issue you reported with Service, plus a couple others |
|
Superseded by #1075, I'll open different PRs to address my other maintainability changes |
2 participants
Public API Changes
None
Description
While the typing work was extensive to represent the RosBridge protocol, there were some things that were overlooked in the migration that meant that the type checking both for the developers of this library, and the types delivered to developers working with the lib weren't exhaustive.
This PR addresses this by refactoring the protocol files:
Apologies for the name changes, I had this left over from another PR I didn't get to complete and the scheme was completely different from yours.
This PR also does some housekeeping on complex methods with large Cyclomatic Complexity, making them easier to read and maintain. In particular,
Service#advertiseandadvertiseAsynchave a common parent function now, joining the common parts together.I also split out some of the longer that were declared right in function parameters, which makes them harder to read and follow when you have a large object in the constructor.
As always, thanks for the consideration and happy to address any comments :)
Fixes #1068
Fixes #1069