Fix Flatten when current_node is created after the flatten #139
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…ing a push And adds a test for this case
Flatten now uses bind! which handles this case well
|
Cool! 👍 The merged commits appear to be still here. I will squash-merge this. |
|
Ok, I can rebase right now if that's easier |
shashi
reviewed
May 16, 2017
| current_node = input.value | ||
| output.parents = (input, current_node) | ||
| bind!(output, current_node, false) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Found a nice bug in
flatten: When the flatten'scurrent_node(the current value of the input Signal{Signal}) is created after the flatten, the flatten won't update when the current_node updates. This is because under normal circumstances, updates to that current_node will only propagate to nodes created after it. So simply having that node as a parent of the flatten, as per the existing implementation, didn't work.The neat fix is to use
bind!which I had previously taken care to handle an equivalent case - when the src node is downstream from (created after) the destination node.N.b. only the last commit is relevant here. The first 3 commits are from #138 which should be merged before this.