feat: added get graph command #408
Conversation
|
LGTM. @eysi09 any thoughts? |
| opts: {}, | ||
| }) | ||
|
|
||
| expect(Object.keys(res.result!).sort()).to.eql(["nodes", "relationships"]) |
There was a problem hiding this comment.
Why is there a sort here? the order should be predictable right?
There was a problem hiding this comment.
Maybe the sort is unnecessary, but I find the key ordering guarantee a bit vague:
So I've always assumed I had to ensure the key order myself when using Object.keys.
There was a problem hiding this comment.
My point is if the function should return a predictable list, then the function should do the sorting not the tests... it hides hidden logic around ordering. (in this test it doesn't matter)
I can see that you are actually trying to get the keys out of the "object" but I guess we should be explicitly checking that the 2 objects are equal?
This test doesn't feel like it tests a valid expected result?
There is a bunch of logic in the dependency-graph.ts that does filtering / sorting but never actually tests tested?
There was a problem hiding this comment.
Without a stable topological sorting algorithm (didn't find a JS lib that implemented one of those), testing the ordering means essentially re-implementing the logic in the test, so I'm skipping that for now (added a TODO comment about this).
I think we should just merge this one for now.
thsig
force-pushed
the
get-graph-command
branch
from
15c4592 to
9513b65
Compare
This command returns a serialized representation of the project's DependencyGraph.
edvald
force-pushed
the
get-graph-command
branch
from
9513b65 to
010353e
Compare
This command returns a serialized representation of the project's
DependencyGraph.