-
Notifications
You must be signed in to change notification settings - Fork 668
Add list level and lacing functionality to the new "If" node. #11598
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
Add list level and lacing functionality to the new "If" node. #11598
Conversation
| Transpose, | ||
| Union, | ||
| InlineConditional, | ||
| ConditionalIf, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
because this enum is public - to avoid reordering the values in the list- this new enum should be added at the end of the list, or assigned an int value greater than the last value in the list.
ie
ConditionalIf = 1000
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you put this one in the middle of the list, even if you set the value, you will still need to explicitly set the following one as well to keep its current value.
https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/language-specification/enums
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will add it to the end of the list which seems safer.
aparajit-pratap
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Set ArgumentLacing to LacingStrategy.Auto in the constructors to enable lacing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
@mjkkirschner @aparajit-pratap Thank you, addressing your comments. |
|
Addressed all comments. |
aparajit-pratap
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@reddyashish thanks for making the changes. I think this is good to go once we have an exhaustive set of tests for the new node.
|
Merging this to feature branch. Adding tests in a new PR. |
* Adding a new "IF" node that would simply return the truevalue or falsevalue based on the test condition. (#11481) * attempt1 at creating partially applied function * change implementation of If NodeModel node * Move the functionality to a new if node that will be renamed. * Update IfTest.cs * Hiding the old If node and renaming to the new one to have the same name. * Throw a warning message when the old node is opened. * add comments. * Update the warning message and move it to the resources file. * Fix failing test Co-authored-by: Aparajit Pratap <aparajit.pratap@autodesk.com> * Add list level and lacing functionality to the new "If" node. (#11598) * Add lacing feature to the "If" node. * Addressing comments * additional comments * Move code into a private function. * Add tests for the new IF node. * Update one of the If tests. * Update the test. * Add the If node icon to the new node. * Changes to the Icon image * Update the IconUrl Co-authored-by: Aparajit Pratap <aparajit.pratap@autodesk.com>

Purpose
This PR is to add list level and lacing functionality to the new PR by introducing a new custom function which would accept "true" and "false" input parameters of arbitrary ranks.
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@aparajit-pratap