Avoid namespace collisions when collapsing graph to custom node#9330
Avoid namespace collisions when collapsing graph to custom node#9330mjkkirschner merged 9 commits intoDynamoDS:masterfrom
Conversation
Not sure how this effects node2code. - may create rewriter directly.
| @@ -674,7 +678,8 @@ private bool InitializeCustomNode( | |||
| nodeGraph.ElementResolver, | |||
There was a problem hiding this comment.
I see the nodeGraph is instantiated from an xml file? Is that why it doesn't get the correct element resolver as we are now using JSON?
There was a problem hiding this comment.
here is where we use the deserialized element resolver when loading a JSON graph and pass it to the home workspace or customNode workspace that was just deserialized.
use it in custmNodeManager collapse method
mjkkirschner
changed the title
|
@aparajit-pratap I've updated this with one potential solution. |
| { | ||
| node.InputSymbol += ":" + typedParameter.Type.Name; | ||
| this.AsLogger().LogError($"{e.Message}: could not generate a short type name for {typedParameter.Type.Name}"); | ||
| } |
There was a problem hiding this comment.
Does this set InputSymbol to the full name even though it is unable to find a short name? If not, it should.
There was a problem hiding this comment.
yep, thats what it does.
| return node; | ||
|
|
||
| // build an idlistnode from the full type name | ||
| var identListNode = CoreUtils.CreateNodeFromString(node.datatype.Name); |
There was a problem hiding this comment.
We can just pass node.TypeAlias to CreateNodeFromString.
There was a problem hiding this comment.
will try that - thanks.
| // build an idlistnode from the full type name | ||
| var identListNode = CoreUtils.CreateNodeFromString(node.datatype.Name); | ||
| if (identListNode == null) | ||
| return null; |
There was a problem hiding this comment.
It might be safer to return node here just in case it's still valid.
| // visit the idlist | ||
| var shortNameNode = identListNode.Accept(this); | ||
| if(shortNameNode == null) | ||
| return null; |
There was a problem hiding this comment.
It might be safer to return node here just in case it's still valid.
| { | ||
| var idNode = (shortNameNode as IdentifierNode); | ||
| resultingNode.TypeAlias = idNode.Value; | ||
| } |
There was a problem hiding this comment.
I think we can just do: resultingNode.TypeAlias = shortNameNode.ToString();
|
|
||
|
|
||
| var resultingNode = new TypedIdentifierNode(); | ||
| resultingNode.Name = node.Name; |
There was a problem hiding this comment.
var resultingNode = new TypedIdentifierNode{
Name = node.Name,
Value = node.Name,
...
}
| node.Name = resultingNode.Name; | ||
| node.Value = resultingNode.Value; | ||
| node.datatype = resultingNode.datatype; | ||
| node.TypeAlias = resultingNode.TypeAlias; |
There was a problem hiding this comment.
Does node have to be passed as a ref parameter in order to directly assign it to resultingNode? Is that why you have modified its properties individually here?
There was a problem hiding this comment.
yep, I found that assigning node = resultantNode did not update the node I passed in, and using a ref parameter requires touching the ASTreplacer base class - so this is not ideal, but it does seem to work.
| parameters = funcDesc.Parameters.ToList(); | ||
|
|
||
| // if the node is an instance member the function won't contain a | ||
| // parameter for this type so we need to geneate a new typedParameter. |
|
@mjkkirschner I think this looks good overall. Couple of other comments you could address here. I will look at the test cases you add before we decide to merge this. Thanks for sticking with this and fixing it :) |
update references add comments to node2code mutation tests
add extra logic to deserialization for symbol node element resolver cache
|
@aparajit-pratap I've moved the I've added a few tests for shortNameReplacer that test the I have yet to run all the tests on self serve CI. |
mjkkirschner
changed the title
test/DynamoCoreTests/CustomNodes.cs
Outdated
| var inputs = CurrentDynamoModel.CurrentWorkspace.Nodes.OfType<Symbol>().ToList(); | ||
| Assert.IsNotNull(inputs); | ||
|
|
||
| var pointParams = inputs; |
There was a problem hiding this comment.
where is pointParams used?
test/DynamoCoreTests/CustomNodes.cs
Outdated
|
|
||
| Assert.IsTrue(inputs.All(t => | ||
| { | ||
| Console.WriteLine(t.InputSymbol); |
There was a problem hiding this comment.
What's the Console.WriteLine for?
test/DynamoCoreTests/CustomNodes.cs
Outdated
| var inputs = CurrentDynamoModel.CurrentWorkspace.Nodes.OfType<Symbol>().ToList(); | ||
| Assert.IsNotNull(inputs); | ||
|
|
||
| var pointParams = inputs; |
|
@mjkkirschner excellent! Some minor comments and then LGTM! |
# Conflicts: # src/DynamoCore/Core/CustomNodeManager.cs # src/DynamoCore/Engine/NodeToCode/NodeToCode.cs # src/DynamoCore/Graph/Nodes/CustomNodes/Function.cs # src/DynamoCore/Graph/Workspaces/NodesToCodeExtensions.cs
…moDS#9330) * this works - but I am hesitant to modify this replacer. Not sure how this effects node2code. - may create rewriter directly. * add a new visitor method for typedIdentifierNode use it in custmNodeManager collapse method * move shortQualifiedNameReplacer update references add comments to node2code mutation tests * add more tests add extra logic to deserialization for symbol node element resolver cache * revert public API break, keep rewriter in dynamoCore but in new file * remove extra usings added previously. * remove debugging code
… (#9360) * this works - but I am hesitant to modify this replacer. Not sure how this effects node2code. - may create rewriter directly. * add a new visitor method for typedIdentifierNode use it in custmNodeManager collapse method * move shortQualifiedNameReplacer update references add comments to node2code mutation tests * add more tests add extra logic to deserialization for symbol node element resolver cache * revert public API break, keep rewriter in dynamoCore but in new file * remove extra usings added previously. * remove debugging code
2 participants
Purpose
Collapsing a graph to a custom node can create errors if there is a namespace conflict in types used in the generated input nodes.
Adds a new visitor to the shortQualifiedNameReplacer and uses it to replace names when creating custom node symbols when collapsing.
Will cherry pick this to 2.1 branch if no issues found.
Declarations
Check these if you believe they are true
*.resxfilesReviewers
@aparajit-pratap
FYIs
@smangarole