refactor: reuse protobuf Vertex definition #164
Conversation
Re-use exisiting protobuf Vertex definition for `packages/object`,
removing the need to maintain a separate TS typedef. This is done by
re-exporting the protobuf-gen typedef with the same name.
Since protobuf makes all its field optional by default since proto3, to
address the undefined warning, a few modification is done:
1. `addToFrontier` now returns a Vertex with a defined `operation`
field.
2. `merge` now ignores vertex with an undefined operation field.
Alternatively we can make `merge` throw or log an error when vertex
with undefined operation is received.
Closes drp-tech#151.
|
Might need to add some tests for the changes I make in |
Co-authored-by: Oak <[email protected]>
| vertices[0].operation && | ||
| vertices[1].operation && |
There was a problem hiding this comment.
why is this needed? you changed correctly the other conditions e.g. vertices[0].operation?....
There was a problem hiding this comment.
Might be me overthinking, but my thought process was so:
In case of vertices 0 is undefined & vertices 1 having undefined value (null will not trigger this), without the first 2 checks, it'll resolve to:
undefined !== v1type && undefined === undefined === true && true
The next statement of vertices[0].operation.type === "add" will then result in false, resulting in. {action: ActionType.DropLeft}.
However my understanding is that if such a case happen that v0 have no ops & v1 have an ops, it should be a no ops since its invalid?
If this is way too much of an edge case then I am happy to remove it.
There was a problem hiding this comment.
in theory, it's impossible for that function to receive an undefined operation there (unless u mess with the hg manually). Although i really like your approach and it makes the code more reliable, we can go with it. Although it'll troublesome in the future, to detect that the linearizeOps is returning undefined/null ops
Re-use exisiting protobuf Vertex definition for
packages/object, removing the need to maintain a separate TS typedef. This is done by re-exporting the protobuf-gen typedef with the same name.Since protobuf makes its message field optional by default since proto3, to address the undefined warning, a few modification is done:
addToFrontiernow returns a Vertex with a definedoperationfield.mergenow ignores vertex with an undefined operation field. Alternatively we can makemergethrow or log an error when vertex with undefined operation is received.Closes #151.