Rewrite graph to use a map data structure#78
Conversation
tchayen
left a comment
tchayen
left a comment
There was a problem hiding this comment.
Hi! Thanks for this very solid work. Especially appreciate tests.
Looks like we have now two PRs aiming to change internal graph representation – #50 and this one. Although progress there stagnated and became partially irrelevant as for example Foam moved on from depending on this extension.
Local graph view is definitely something worth considering. Looking forward to merging this PR, need to give it more thought first.
Adding some comments with requests for changes.
src/test/suite/graph.test.ts
Outdated
| import { assert } from 'chai'; | ||
| import { Graph, Node } from '../../types'; | ||
|
|
||
| describe('Graph', function() { |
There was a problem hiding this comment.
Please use Prettier as in other files of the repository.
| export type Node = { | ||
| export type D3Node = { | ||
| id: string; | ||
| path: string; | ||
| label: string; | ||
| }; | ||
|
|
||
| export type Graph = { | ||
| nodes: Node[]; | ||
| edges: Edge[]; | ||
| export type D3Edge = { | ||
| source: string; | ||
| target: string; |
There was a problem hiding this comment.
Are those two changes reflected in other places? Maybe it's dead code and might as well be removed?
There was a problem hiding this comment.
These types are still used in the return from Graph.toD3Graph(), which is what is called to get the payload that is sent to the D3 webview.
jmatth
force-pushed
the
graph-data-structure
branch
from
f9a386c to
448514b
Compare
2 participants
This should address #38. Doesn't add any functionality on its own, but I'm looking into adding a local graph view (only show the current file and any nodes connected to it) and this is a prerequisite for that to run with any efficiency.