Fix branchedResetPropagation #57

nif · 2024-02-12T19:46:50Z

nif commented

2024-02-12 19:46:50 +00:00

BREAKING CHANGE - Forces branches to be directed

Signed-off-by: nif nif@incremental.social

BREAKING CHANGE - Forces branches to be directed Signed-off-by: nif <nif@incremental.social>

nif added 1 commit 2024-02-12 19:46:51 +00:00

Fix branchedResetPropagation

Run Tests / test (pull_request) Has been cancelled

Details

5e32fa4985

BREAKING CHANGE - Forces branches to be directed

Signed-off-by: nif <nif@incremental.social>

thepaperpilot requested changes 2024-02-13 14:23:41 +00:00

thepaperpilot left a comment

I wrote some regression tests, and they failed before this PR and passed after, so the logic looks good but I had some notes about the code itself.

src/features/trees/tree.ts Outdated

					
				@ -367,2 +341,2 @@

				            visitedNodes.push(...currentNodes);

				        }

				    const links = unref(tree.branches);

				    if (links === undefined) return;

thepaperpilot commented

2024-02-13 14:21:47 +00:00

instead of === undefined, can you use == null? That matches the rest of the codebase and has subtly different behavior.

instead of `=== undefined`, can you use `== null`? That matches the rest of the codebase and has subtly different behavior.

nif commented

2024-02-13 15:36:12 +00:00

You very well could, I didn't realise that == null tested for undefined

You very well could, I didn't realise that `== null` tested for `undefined`

src/features/trees/tree.ts Outdated

					
				@ -369,0 +343,4 @@

				    let reset: GenericTreeNode[] = [];

				    let current = [resettingNode];

				    while (current.length != 0) {

				        let next: GenericTreeNode[] = [];

thepaperpilot commented

2024-02-13 14:20:46 +00:00

The linter says next, node, and link should all be const

The linter says next, node, and link should all be `const`

nif commented

2024-02-13 15:36:53 +00:00

that's probably because i use Array.prototype.concat and Array.prototype.push instead of reassigning them

that's probably because i use `Array.prototype.concat` and `Array.prototype.push` instead of reassigning them

src/features/trees/tree.ts Outdated

					
				@ -369,0 +351,4 @@

				                link.endNode.reset?.reset();

				            }

				        };

				        reset = reset.concat(current);