Graph grouping/collapsing code tidy #2129
Conversation
| const indexSearch = Object.values(this.cylcTree.$index).find( | ||
| (node) => node.name === family && node.tokens.cycle === cycle | ||
| ) |
There was a problem hiding this comment.
(unrelated) Walking over the full index is really slow, it can contain nodes from other workflows.
| const indexSearch = Object.values(this.cylcTree.$index).find( | ||
| (node) => node.name === family && node.tokens.cycle === cycle | ||
| ) | ||
| if (!indexSearch) continue |
There was a problem hiding this comment.
Does linting really allow omitting the braces?
There was a problem hiding this comment.
Yes for control statements on the same line as the condition
| for (const { $edges } of this.workflows) { | ||
| for (const { tokens } of $edges || []) { |
There was a problem hiding this comment.
Maybe a bit to clever, the previous syntax is much more obvious.
There was a problem hiding this comment.
Eh, destructuring is a core feature of JS that IMO everyone should get used to
src/views/Graph.vue
Outdated
| } | ||
| } | ||
| return ret | ||
| return new Map(ret) |
There was a problem hiding this comment.
Why build an array then convert to a map?
There was a problem hiding this comment.
Good question... made this a Map from the start
src/views/Graph.vue
Outdated
| nodes.map(n => n.id).reduce((x, y) => { return x + y }) + | ||
| (edges || []).map(n => n.id).reduce((x, y) => { return x + y }, 1) | ||
| nodes.map(n => n.id).reduce((x, y) => x + y) + | ||
| Array.from(edges.keys()).reduce((x, y) => x + y, 1) |
There was a problem hiding this comment.
Turning the map into an array might be a bit expensive.
There was a problem hiding this comment.
Seeing as originally it was doing an Array.prototype.map() followed by an Array.prototype.reduce(), it shouldn't be any more expensive as there were 2 loops before. However, newer browsers support edges.keys().reduce() directly, so we could do something like
const edgesReducer = (x, y) => x + y
// If browser supports Iterator.prototype.reduce(), use it:
edges.keys().reduce?.(edgesReducer) ?? Array.from(edges.keys()).reduce(edgesReducer)to take advantage of it on the browsers that support it?
Adds ugliness to code however
|
LGTM, but doesn't work right. I tested with the complex workflow, the graph didn't produce any errors, but it also didn't layout and left all of the tasks in a long line. When I tried to group by cycle I got this error in the console: |
|
As discussed in person, I can't reproduce the problem with the same workflow |
Follow-up to #1810
Partly addresses #1130
Check List
CONTRIBUTING.mdand added my name as a Code Contributor.?.?.xbranch.