Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Exclude build dependencies when counting inputs to new nested node #1946

Merged
merged 3 commits into from
Aug 7, 2024

Conversation

glopesdev
Copy link
Member

Since build dependencies always affect their target node and are not considered proper inputs to operators, they should be excluded when estimating number of inputs to a newly created nested node.

Fixes #1792

@glopesdev glopesdev added the fix Pull request that fixes an issue label Aug 2, 2024
@glopesdev glopesdev added this to the 2.9 milestone Aug 2, 2024
@glopesdev glopesdev requested a review from PathogenDavid August 2, 2024 11:17
@PathogenDavid
Copy link
Member

How difficult would it be to add a test for this? This fix seems like a good candidate for a test but I know testing stuff in the UI layer can be a pain

@glopesdev
Copy link
Member Author

How difficult would it be to add a test for this? This fix seems like a good candidate for a test but I know testing stuff in the UI layer can be a pain

@PathogenDavid Thanks for the remainder, definitely important to keep doing this, especially for these edge cases, and especially for preparing the migration to a different UI layer.

It turned out to be a bit more difficult than expected, but not because of the UI. The main obstacle was I had to add testing infrastructure for branches, and also had to remember that the initial state of the workflow must be topologically sorted to ensure reversibility of undo/redo operations, so was good to make that explicit at least in a comment.

I think now TestWorkflow can be used to build pretty much any workflow programmatically so can almost be considered an API for a scripting editor or DSL.

@glopesdev glopesdev merged commit 70520d9 into bonsai-rx:main Aug 7, 2024
10 checks passed
@glopesdev glopesdev deleted the issue-1792 branch August 7, 2024 18:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix Pull request that fixes an issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Replacing a node with a group workflow when it receives a visualizer mapping generates a bogus source node
2 participants