comments are not serialized with custom node input nodes #8825

mjkkirschner · 2018-05-03T21:49:24Z

Purpose

@andydandy74 reported missing comments in his converted clockwork package.
~~This PR serializes type parameter comments into the nodeView of input(symbol) nodes, using as simple as a converter as I could write.~~

~~we use the new converter when saving the workspaceViewModel.~~

At load time, we pull the comment out of the nodeView if the node we're loading is an input node, and then we construct a new type parameter using that comment and existing type paramter, and then set the inputSymbol equal to the string representation of that new type parameter.

This PR serializes the typedParameter.Summary as Description using the existing TypedParamterConverter and adds some tests. It appears that if we load an existing custom node with no Description in the parameter we get an emptyString back from the symbol node - seems reasonable to me.

comments are always appended to the start of the input symbol and they are always wrapped with block comments as the parser throws away everything but the comment's string value. - ie

for //a comment we can only serialize a comment

sample serialization:

 "Parameter": {
        "Name": "inputName",
        "TypeName": "string",
        "TypeRank": 0,
        "DefaultValue": "\"a def string\"",
        "Description": " commentsssss"
      },

TODO:

This PR needs tests.

Declarations

Check these if you believe they are true

The code base 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.

Reviewers

@QilongTang
@ramramps

FYIs

@smangarole @andydandy74

needs tests

ramramps · 2018-05-04T14:04:24Z

@mjkkirschner Does summary serializes in NodeView.Parameter? or as Summary in NodeView?

mjkkirschner · 2018-05-04T14:23:32Z

@ramramps Parameter.Summary serializes to NodeView.Summary but we could name it anything.

QilongTang · 2018-05-04T15:10:11Z

src/DynamoCore/Graph/Workspaces/WorkspaceModel.cs

 using Dynamo.Engine;
 using Dynamo.Scheduler;
 using Newtonsoft.Json.Linq;
+using Dynamo.Library;


Added this for Nodes.CustomNodes.Symbol type check?

ramramps · 2018-05-04T15:12:35Z

@mjkkirschner is there a reason for not adding this to Parameter block?
"Parameter": {
"Name": "var1",
"TypeName": "System.DateTime",
"TypeRank": 0,
"DefaultValue": null
},
"

QilongTang · 2018-05-04T15:12:41Z

src/DynamoCoreWpf/ViewModels/Core/Converters/SerializationConverters.cs

+            //if the nodeView's model is a symbol node add some extra data.
+            if ((value as NodeViewModel).NodeModel is Symbol)
+            {
+                jobject.Add("Summary", ((value as NodeViewModel).NodeModel as Symbol).Parameter.Summary);


Can you check if empty summary will be kept and serialized as empty string? Just curious

gregmarr · 2018-05-04T15:21:19Z

I asked the same thing, should we add another field to Parameter? I'm fine with that.

QilongTang · 2018-05-04T15:23:59Z

@gregmarr @ramramps Adding to the param block seems to be cleaner implementation but I thought we agreed compute block only contains stuff meant to be must have for computation?

gregmarr · 2018-05-04T15:28:26Z

It's better than adding it to the TypeName field as a comment, which is what this looks like it is doing. If we add Parameter.Description it matches Port.Description. I guess the alternative would be adding it under View.NodeModel.

mjkkirschner · 2018-05-04T15:30:22Z

@gregmarr this PR is meant to go into a release quite soon, not just master. Does that cause any issues if we move this to parameter?

Summary already exists on paramter so it's very simple AFAIK to just serialize summary into the parameter...

I'm not sure what you mean here
It's better than adding it to the TypeName field as a comment, which is what this looks like it is doing. We do a funny thing when setting the inputSymbol, the string is parsed back to a new TypedParamter...

gregmarr · 2018-05-04T15:31:15Z

I'm talking about adding it to the JSON Schema.

mjkkirschner · 2018-05-04T15:32:25Z

right, so we would get:

"Parameter": {
"Name": "var1",
"TypeName": "System.DateTime",
"TypeRank": 0,
"DefaultValue": null,
"Summary" : "a comment"
},

@ramramps @QilongTang are we okay with that?

mjkkirschner · 2018-05-04T15:33:45Z

@gregmarr would this change to the schema make it into production?

gregmarr · 2018-05-04T15:34:42Z

Can we use Description to match Port, Graph, InputData and OutputData?

QilongTang · 2018-05-04T15:34:46Z

@mjkkirschner I am fine with it given that it is much cleaner, I just thought we do not want to implement it that way in the first place because this looks like a schema change

mjkkirschner · 2018-05-04T15:36:00Z

Yep, well I was trying to avoid them because I believe they cause work for @gregmarr and we have to reason about what will be in production at what time, but If @gregmarr is fine with it I think its cleaner.

gregmarr · 2018-05-04T15:36:03Z

We're generally okay with additions to the schema. Since it's adding data that isn't otherwise there, it won't hurt anything. We can have this in CoGS dev immediately, staging and production fairly soon.

gregmarr · 2018-05-04T15:36:38Z

Well, I'm the one who didn't include it in the schema in the first place, so I'm fine with it. :)

mjkkirschner · 2018-05-04T15:36:57Z

@ramramps @gregmarr @QilongTang I will change this implementation to serialize like this:

"Parameter": {
"Name": "var1",
"TypeName": "System.DateTime",
"TypeRank": 0,
"DefaultValue": null,
"Description" : "a comment"
},```

ramramps · 2018-05-04T15:37:47Z

"Description" = "Summary" ?

mjkkirschner · 2018-05-04T15:38:11Z

yes @gregmarr requested that above to match port.

ramramps · 2018-05-04T15:38:58Z

Thanks @mjkkirschner just read the comments.. I am fine with Description...

fix comment

mjkkirschner · 2018-05-04T20:02:50Z

@QilongTang @ramramps made the changes PTAL.

mjkkirschner · 2018-05-04T20:03:24Z

Any other tests you can think of that I should add here?

ramramps · 2018-05-04T20:19:28Z

src/DynamoCore/Graph/Workspaces/SerializationConverters.cs

            writer.WritePropertyName("DefaultValue");
            writer.WriteValue(((TypedParameter)value).DefaultValueString);
+            writer.WritePropertyName("Description");
+            writer.WriteValue(((TypedParameter)value).Summary);


ramramps · 2018-05-04T20:21:06Z

This looks better [ graph serialization / deserilization] . thanks @mjkkirschner
LGTM

* first commit - seems to be working needs tests * get rid of nodeViewModel converter * add tests fix comment

* swtich registration of js object to async verison - we only have access to methods here, and they return promises. * review comments * Update library.html review comments * comments are not serialized with custom node input nodes (#8825) * first commit - seems to be working needs tests * get rid of nodeViewModel converter * add tests fix comment

first commit - seems to be working

69bb9bd

needs tests

QilongTang reviewed May 4, 2018

View reviewed changes

mjkkirschner added the Requires Changes label May 4, 2018

mjkkirschner added 2 commits May 4, 2018 13:40

get rid of nodeViewModel converter

914b128

add tests

3ea3a7f

fix comment

mjkkirschner added PTAL Please Take A Look 👀 and removed Requires Changes labels May 4, 2018

ramramps reviewed May 4, 2018

View reviewed changes

mjkkirschner changed the title ~~WIP - comments are not serialized with custom node input nodes~~ comments are not serialized with custom node input nodes May 7, 2018

mjkkirschner mentioned this pull request May 7, 2018

cherry pick missing comments in customNodes #8835

Closed

QilongTang added LGTM and removed PTAL Please Take A Look 👀 labels May 7, 2018

QilongTang merged commit 70bcfb2 into DynamoDS:master May 7, 2018

QilongTang pushed a commit that referenced this pull request May 7, 2018

comments are not serialized with custom node input nodes (#8825)

5425096

* first commit - seems to be working needs tests * get rid of nodeViewModel converter * add tests fix comment

QilongTang mentioned this pull request May 7, 2018

Cherry picks #8843

Merged

7 tasks

comments are not serialized with custom node input nodes #8825

comments are not serialized with custom node input nodes #8825

Uh oh!

Conversation

mjkkirschner commented May 3, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Reviewers

FYIs

Uh oh!

ramramps commented May 4, 2018

Uh oh!

mjkkirschner commented May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

QilongTang May 4, 2018

Choose a reason for hiding this comment

Uh oh!

ramramps commented May 4, 2018

Uh oh!

QilongTang May 4, 2018

Choose a reason for hiding this comment

Uh oh!

gregmarr commented May 4, 2018

Uh oh!

QilongTang commented May 4, 2018

Uh oh!

gregmarr commented May 4, 2018

Uh oh!

mjkkirschner commented May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gregmarr commented May 4, 2018

Uh oh!

mjkkirschner commented May 4, 2018

Uh oh!

mjkkirschner commented May 4, 2018

Uh oh!

gregmarr commented May 4, 2018

Uh oh!

QilongTang commented May 4, 2018

Uh oh!

mjkkirschner commented May 4, 2018

Uh oh!

gregmarr commented May 4, 2018

Uh oh!

gregmarr commented May 4, 2018

Uh oh!

mjkkirschner commented May 4, 2018

Uh oh!

ramramps commented May 4, 2018

Uh oh!

mjkkirschner commented May 4, 2018

Uh oh!

ramramps commented May 4, 2018

Uh oh!

mjkkirschner commented May 4, 2018

Uh oh!

mjkkirschner commented May 4, 2018

Uh oh!

ramramps May 4, 2018

Choose a reason for hiding this comment

Uh oh!

ramramps commented May 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

mjkkirschner commented May 3, 2018 •

edited

Loading

mjkkirschner commented May 4, 2018 •

edited

Loading

mjkkirschner commented May 4, 2018 •

edited

Loading

ramramps commented May 4, 2018 •

edited

Loading