Migrating formula node deserialization to code block nodes#14196
Migrating formula node deserialization to code block nodes#14196aparajit-pratap merged 17 commits intoDynamoDS:masterfrom
Conversation
aparajit-pratap
changed the title
aparajit-pratap
requested review from
QilongTang,
mjkkirschner,
pinzart90 and
sm6srw
aparajit-pratap
changed the title
mjkkirschner
left a comment
mjkkirschner
left a comment
There was a problem hiding this comment.
looking pretty good, a few questions and an interesting failure in the build - I guess NCalc brought in a dependency we didn't know we had...
Net6 has transitive dependencies active by default. Thats what I'm assuming is going on.
| internal string ConvertFormulaToDS(string formula) | ||
| { | ||
| CodeBlockNode cbn = null; | ||
| var match = ifRgx.Match(formula); |
There was a problem hiding this comment.
is it simpler to use the DS parser to check for conditional statements - or theres a high chance of failure trying to parse the entire formula string?
There was a problem hiding this comment.
This syntax for the if statement will fail to parse using the DS parser, which is why I'm splitting it up using regex into parts that can be sent individually to the DS parser.
There was a problem hiding this comment.
When the parser fails, it will throw an exception and the entire deserialization of the formula node will be skipped, which is undesirable. We want to convert it to DS as much as possible, if not keep the node and issue an appropriate warning to the user saying they need to manually convert it. In this case, I'm just copying the original string in the formula node and pasting it in a code block node with a warning to that effect.
|
":white_check_mark: Bin-Diff Issue Resolved." |
aparajit-pratap
changed the title
| inputAstNodes)) | ||
| }; | ||
| newNode.Attributes["CodeText"].Value = convertedCode; | ||
| //(node as CodeBlockNodeModel).FormulaMigrationWarning(Resources.FormulaMigrated); |
There was a problem hiding this comment.
is this not accessible from here?
There was a problem hiding this comment.
No, over here I don't have a handle to the Nodemodel, just the xml node. Trying to figure this one out as the last thing to do here.
|
4 failures in DynamoAnalyticsTests and 1 with PreferenceSettingsTests that are unrelated to this PR, they also occur in master currently. Merging. |
3 participants
Purpose
Attempt at migrating formula node deserialization to code block nodes.The regex expressions used so far do not support arbitrarily nested expressions just yet, for example, expressions like
if(sin(90+10) > log10(100), 45, tan(90)), are not translated properly into DS for CBNs.Auto-migration of Formula node to Codeblock node during workspace deserialization. In cases where the auto-conversion, fails, the unchanged formula code is copied to a CBN with an appropriate warning. Even in cases where there is a successful conversion to a codeblock node, a warning is issued on the converted node indicating that it has been created by migrating to a Formula node.
This PR removes the dependency on ncalc and deprecates both Formula
nodemodelnode as well asEvaluateFormulaZT node.TODO:
Unit tests to test migrationCleanup (Formula.cs file needs to be deleted, not just removed)One test has been commented out, it needs to be updatedFailed migration case:
Declarations
Check these if you believe they are true
*.resxfilesRelease Notes
(FILL ME IN) Brief description of the fix / enhancement. Mandatory section
Reviewers
(FILL ME IN) Reviewer 1 (If possible, assign the Reviewer for the PR)
(FILL ME IN, optional) Any additional notes to reviewers or testers.
FYIs
(FILL ME IN, Optional) Names of anyone else you wish to be notified of