Skip to content

Conversation

@reddyashish
Copy link
Contributor

@reddyashish reddyashish commented Apr 8, 2021

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

  • The codebase is in a better state after this PR
  • Is documented according to the standards
  • The level of testing this PR includes is appropriate
  • User facing strings, if any, are extracted into *.resx files
  • All tests pass using the self-service CI.
  • Snapshot of UI changes, if any.
  • Changes to the API follow Semantic Versioning and are documented in the API Changes document.
  • This PR modifies some build requirements and the readme is updated

Reviewers

@aparajit-pratap

@reddyashish reddyashish added the WIP label Apr 8, 2021
Transpose,
Union,
InlineConditional,
ConditionalIf,
Copy link
Member

@mjkkirschner mjkkirschner Apr 10, 2021

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

Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

@aparajit-pratap aparajit-pratap left a 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.

Copy link
Contributor

@aparajit-pratap aparajit-pratap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think after my comments are addressed we'll have all that we need for the new If node:

  • partial function support
  • lacing
  • list at level 🥳

I've tested this locally:
image

@reddyashish
Copy link
Contributor Author

reddyashish commented Apr 12, 2021

@mjkkirschner @aparajit-pratap Thank you, addressing your comments.

@reddyashish reddyashish removed the WIP label Apr 12, 2021
@reddyashish
Copy link
Contributor Author

Addressed all comments.

@reddyashish reddyashish changed the title Add lacing functionality to the new "If" node. Add list level and lacing functionality to the new "If" node. Apr 12, 2021
Copy link
Contributor

@aparajit-pratap aparajit-pratap left a 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.

@reddyashish
Copy link
Contributor Author

Merging this to feature branch. Adding tests in a new PR.

@reddyashish reddyashish merged commit 297326b into DynamoDS:RefactoredIfNode Apr 13, 2021
@reddyashish reddyashish mentioned this pull request Jun 2, 2021
8 tasks
reddyashish added a commit that referenced this pull request Jun 8, 2021
* 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>
@reddyashish reddyashish deleted the aparajit-pratap-ifNode branch November 23, 2022 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants