-
Notifications
You must be signed in to change notification settings - Fork 11
Add graph extraction helpers to make it easier to use NVL #517
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
Conversation
|
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.
Looks good. I had some small comments, but after you decide on those it should be fine to merge
packages/query-tools/src/cypher-execution/extract-unique-nodes-and-relationships.test.ts
Outdated
Show resolved
Hide resolved
/** | ||
* Result type containing deduplicated nodes and relationships extracted from Neo4j records. | ||
*/ | ||
export type DeduplicatedBasicNodesAndRels = { |
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.
What does the "Basic" part here mean?
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.
Orginally we named the nodes BasicNode
to avoid clashing with the web browser api Node and to distinguish from the Nodes used in NVL. Here we don't have that problem, so I'll drop the basic
part for now
}: { nodeLimit?: number; keepDanglingRels?: boolean } = {}, | ||
): DeduplicatedBasicNodesAndRels => { | ||
let limitHit = false; | ||
if (records.length === 0) { |
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.
This check looks like it isnt needed. Do you think it makes the code clearer/gives a worthwhile optimization, or what do you think about removing it and instead having a test like:
test('should not break on empty bolt records', () => {
const { nodes, relationships, limitHit } = extractUniqueNodesAndRels([]);
if (
nodes === undefined ||
relationships === undefined ||
limitHit === undefined
) {
throw new Error('Invalid return values on empty array of paths');
}
expect(nodes.length).toBe(0);
expect(relationships.length).toBe(0);
expect(limitHit).toBe(false);
});
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.
I added a test similar to this now, thanks 👍
…-and-relationships.test.ts Co-authored-by: Isak Nilsson <[email protected]>
No description provided.