FlorianHockmann commented on code in PR #2599:
URL: https://github.com/apache/tinkerpop/pull/2599#discussion_r1622405611
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/graph.ts:
##########
@@ -73,20 +77,26 @@ export class Vertex<T extends Record<string, any> =
Record<string, any>> extends
}
}
-export class Edge<T extends Record<string, any> = Record<string, any>> extends
Element {
+export class Edge<
+ TOutVertex extends Vertex = Vertex,
+ TLabel extends string = string,
+ TInVertex extends Vertex = Vertex,
+ TProperties extends Record<string, any> = Record<string, any>,
+ TId extends number | string = number,
Review Comment:
I think this is too restrictive. Providers can use any type they like for
the ID. JanusGraph for example has its own type.
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/io/type-serializers.ts:
##########
@@ -349,9 +349,9 @@ export class EdgeSerializer extends TypeSerializer<g.Edge> {
[valueKey]: {
id: this.writer.adaptObject(item.id),
label: item.label,
- outV: this.writer.adaptObject(item.outV.id),
+ outV: this.writer.adaptObject(item.outV?.id),
Review Comment:
Can `outV` really be `undefined`? And shouldn't users get an exception here
if it is?
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/graph.ts:
##########
@@ -43,27 +42,32 @@ export class Graph {
}
}
-class Element {
+class Element<TLabel extends string = string, TId = any> {
constructor(
- readonly id: string,
- readonly label: string,
+ readonly id: TId,
+ readonly label: TLabel,
Review Comment:
Can you please explain the reasoning behind making this generic? Isn't
`label` always a `string`?
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/graph.ts:
##########
@@ -119,6 +134,13 @@ export class VertexProperty<T1 = any, T2 = any> extends
Element {
}
}
+export type VertexProperties<
+ TLabel extends string = string,
+ TValue = any,
+ TProperties extends Record<string, any> = Record<string, any>,
+ TId = any,
+> = [VertexProperty<TLabel, TValue, TProperties, TId>,
...Array<VertexProperty<TLabel, TValue, TProperties, TId>>];
Review Comment:
Sorry, but I don't know this notation and it's a bit hard to find any
information on it as I don't even know what to search for here 😅
Would you be so kind to provide me with some documentation for this kind of
type definition or maybe the name of the concept for this so I can search for
it myself? :-)
##########
gremlin-javascript/src/main/javascript/gremlin-javascript/lib/structure/graph.ts:
##########
@@ -73,20 +77,26 @@ export class Vertex<T extends Record<string, any> =
Record<string, any>> extends
}
}
-export class Edge<T extends Record<string, any> = Record<string, any>> extends
Element {
+export class Edge<
+ TOutVertex extends Vertex = Vertex,
Review Comment:
Why `TOutVertex extends Vertex`? Why not simply `Vertex`?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
