DYN-5965: introduce new string from nodes that accept numeric format specifiers #14369

mjkkirschner · 2023-09-01T21:29:50Z

Purpose

this PR does the following:

adds some new overloads for the string conversion methods that the String From Object and String From Array nodes call internally.
modifies the built-in methods and string from nodes to accept a second optional parameter formatSpecifier the nodes pass these specifiers down in the case of integer or double to string calls.
adds two new nodes String From Object And Format and String From Array And Format.
adds DynamoPreferencesNumberFormat as a special format that uses the preferences to set the specifier - maybe a simpler name for this DYNFORMAT? I am still on the fence about this.
When we encounter a format exception - for example if you user tries to apply a binary format string to a floating point number (not allowed) - we catch the exception, log a warning on the node, and return the string "Format specifier was invalid." instead of the formatted string.
- If this occurs to ANY data in an array, the entire array is not formatted.
adds extended help docs for these two new nodes.
new nodes are marked experimental.
Introduces a new experimental state for nodes, if this is true a node will display an experimental glyph in the UI and there will be some extra text in the description.
Introduces a feature flag to control if the glyph should be displayed.

support all c# numeric specifiers:

a new experimental glyph is added - we also display this glyph on nodes that are in the experimental category in the preferences - this is done with namespace matching.

we use the standard dotnet experimental attribute to control the experimental state - we also have a tooltip explaining what this means. Package authors can use this and it integrates into c# - if someone tries to use an experimental API it is a compile time error and they need to explicitly disable the warning to compile their c# code.

also add a bit of text to extended description of the node in the library.

extended docs for the new string format nodes are added and link to the super detailed c# docs.

TODO:

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
This PR contains no files larger than 50 MB

Release Notes

add String From nodes that allows control over format of generated string using format specifiers.
standard c# numeric specifiers are allowed. https://learn.microsoft.com/en-us/dotnet/standard/base-types/standard-numeric-format-strings
in addition a custom Dynamo specifier is allowed that uses the user's current number format preferences in the dynamo preference. Using this is discouraged. DynamoPreferencesNumberFormat. (this is currently undocumented) I'd like insight from team members and ux if it makes sense.
Dynamo now also supports the system.diagnostics.codeanalysis.experimental attribute https://learn.microsoft.com/en-us/dotnet/api/system.diagnostics.codeanalysis.experimentalattribute?view=net-8.0 and will display a small experimental symbol on experimental nodes as well as adding some information to the description for these nodes.

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

…ynamoDS#14344) * fixing mem leaks in progress * fix test and remove todo

add two new tests

github-actions · 2024-10-29T12:55:22Z

UI Smoke Tests

Test: success. 11 passed, 0 failed.
TestComplete Test Result
Workflow Run: UI Smoke Tests
Check: UI Smoke Tests

…blockdisplay

review comments

Amoursol · 2024-11-08T14:51:28Z

@mjkkirschner @Jingyi-Wen color choice of the icon is a bit faded. Also green suggests good - when experimental should be neutral. Suggest switching to Blue here which is our "Informational" color.

Also, styling of the hover tooltip is outdated @mjkkirschner

Amoursol · 2024-11-08T14:51:56Z

@mjkkirschner could we also replace the Dynamo Icon in the Library with the Experimental one?

mjkkirschner · 2024-11-08T15:00:38Z

Also, styling of the hover tooltip is outdated @mjkkirschner

can you give me an example tooltip to checkout that has the correct styling?

@mjkkirschner could we also replace the Dynamo Icon in the Library with the Experimental one?

I can look into this - if it's easy will do it otherwise will file an improvement.

I can change the color as long as I can find the info state color in the resource dictionary - @Jingyi-Wen any idea what else it's used for?

mjkkirschner · 2024-11-08T16:14:38Z

I can look into swapping the icon- if it's easy will do it otherwise will file an improvement.

actually, I don't think this such a great idea - we can control it from the feature flag, but I see two big downsides:

Existing experimental nodes that have nice icons will now not show them in the library - for example the paneling nodes. Or when tsplines was experimental we would not have seen the icons - if we imagined that we did another tsplines like integration today and marked it experimental.
If I am writing an experimental package or node I now don't have a way to test my icon easily - I guess locally I can swap the attribute out, but it's kind of a pain.

I will still evaluate how difficult it is to do technically, but it might benefit from ux thinking about it a bit more. @Jingyi-Wen - maybe we only swap it out if the icon is not the default... or perhaps communicating that the node is experimental is more important than the icon - I'm not sure how to make that call.

aparajit-pratap · 2024-11-08T17:13:09Z

src/Engine/ProtoCore/Utils/StringUtils.cs

@@ -8,6 +8,7 @@ namespace ProtoCore.Utils
 {
    public static class StringUtils
    {
+        //TODO consider removing this option. It makes sharing harder.


This sort of looks similar to the scale factor setting issue where we have one in preferences and another setting in the workspace (graph), where the latter overrides the former. I would have suggested adding this format specifier at the graph level as well but I don't think that's needed if we're going to deprecate the old string nodes at some point.

doc/distrib/NodeHelpFiles/CoreNodeModels.FormattedStringFromArray.md

aparajit-pratap · 2024-11-08T17:32:43Z

src/Libraries/CoreNodeModels/String.cs

@@ -104,8 +107,6 @@ public FormattedStringFromObject() : base("__ToStringFromObjectAndFormat")
    [OutPortTypes("string")]
    [IsDesignScriptCompatible]
    [AlsoKnownAs("DSCoreNodesUI.StringNodes.FromObject", "DSCoreNodesUI.FromObject")]
-    [Obsolete("this node is obsolete, please use the version of string from _ with numeric format option")]
-    [NodeDeprecated]


Why aren't we deprecating this?

My reasoning was as follows:

Since I am marking the new nodes experimental, I did not want to also mark the old nodes as obsolete (hiding them from the lib) - then it would not be clear to users how to proceed in a safe / future proof way.

I'd like to propose we follow this pattern when introducing new nodes (that replace existing ones) - if we are not so sure about the design.

phase 1:

Introduce new nodes as experimental.

Old nodes are untouched.

phase 2:

gather feedback on experimental nodes.

experimental nodes can be changed at any time.

phase 3:

experimental attribute is removed from new nodes.

old nodes are marked obsolete

phase 4:

old nodes are removed in a major release.

Jingyi-Wen · 2024-11-08T17:51:43Z

I can look into swapping the icon- if it's easy will do it otherwise will file an improvement.

actually, I don't think this such a great idea - we can control it from the feature flag, but I see two big downsides:

Existing experimental nodes that have nice icons will now not show them in the library - for example the paneling nodes. Or when tsplines was experimental we would not have seen the icons - if we imagined that we did another tsplines like integration today and marked it experimental.

If I am writing an experimental package or node I now don't have a way to test my icon easily - I guess locally I can swap the attribute out, but it's kind of a pain.

I will still evaluate how difficult it is to do technically, but it might benefit from ux thinking about it a bit more. @Jingyi-Wen - maybe we only swap it out if the icon is not the default... or perhaps communicating that the node is experimental is more important than the icon - I'm not sure how to make that call.

@mjkkirschner I agree with this, experimental is more of a state instead of a symbol for the node itself. I'd suggest not swapping the icon in library
The blue color we use for info state is #38ABDF. Graph node manager has the color :)
Also, I suggest changing the tooltip wording to "This node is currently in the experimental state. Its behavior, name, and signature are subject to change."

aparajit-pratap · 2024-11-08T17:56:23Z

src/DynamoCore/PublicAPI.Unshipped.txt

@@ -2782,6 +2782,7 @@ static Dynamo.Properties.Resources.DescriptionResource1.get -> string
 static Dynamo.Properties.Resources.DirectoryNotFound.get -> string
 static Dynamo.Properties.Resources.DisplayEngineFailureMessageDescription.get -> string
 static Dynamo.Properties.Resources.DllLoadException.get -> string
+static Dynamo.Properties.Resources.DocsExperimentalPrefixMessage.get -> string


Are we still seeing these API analysis warnings in PRs?

I expected to see them but lately I've not been seeing them. I wonder if something changed. @avidit ?

I saw them in the github build checks

aparajit-pratap · 2024-11-08T18:04:10Z

src/DynamoCoreWpf/ViewModels/Core/NodeViewModel.cs

@@ -641,7 +643,16 @@ public bool IsFrozenExplicitly
                return false;
            }
        }
-
+        [Experimental("NVM_ISEXPERIMENTAL_GLPYH")]


Does the glyph need to be experimental?

No, but it has to be public AFAK for wpf binding and was not sure we'd want to support it. Experimental experimental mode ;)

aparajit-pratap

Minor comments and some questions. LGTM.

QilongTang · 2024-11-11T13:02:49Z

src/DynamoCore/Configuration/PreferenceSettings.cs

+        internal static readonly IEnumerable<string> InitialExperimentalLib_Namespaces =
+        [
+            //TODO remove tsplines - it's no longer experimental.
+            "ProtoGeometry.dll:Autodesk.DesignScript.Geometry.TSpline",


Thanks, I thought I removed the TSpline feature flag check would be enough, I guess not..

QilongTang · 2024-11-11T13:04:12Z

src/DynamoCore/Library/FunctionDescriptor.cs

@@ -145,7 +146,12 @@ public FunctionDescriptorParams()
        /// <summary>
        ///     Indicates if the lacing strategy is disabled on the function
        /// </summary>
-        public bool IsLacingDisabled { get; set; } 
+        public bool IsLacingDisabled { get; set; }
+        //TODO - should this somehow contain more info - ExperimentalInfo{IsExperimental, ExperimentalMessage/url}?}


How would the message be surfaced in your opinion if it exists?

kind of like how the underlying system.diagnostics.codeanalysis.experimental attribute surfaces them. In this case it shows the errors in the errors tab of visual studio as compile time warnings and generates a url given a base url format and the experimental diagnostic id.

we could do something similar, show info states on nodes pointing to the docs browser, though it would be much more useful if you get a link to some dynamic documentation that we update, for example in the forum or primer.

For now I will just leave it as a TODO and file a follow-up to consider it.

see:
https://learn.microsoft.com/en-us/dotnet/fundamentals/syslib-diagnostics/experimental-overview

mjkkirschner · 2024-11-11T21:37:34Z

I think I have addressed all review comments, I will merge after I see a passing test result - at the moment a date time test is failing, I think @pinzart90 was trying to improve in another pr.

mjkkirschner · 2024-11-11T21:57:51Z

passed here:
https://master-5.jenkins.autodesk.com/job/Dynamo/job/DynamoSelfServe/job/pullRequestValidation/16776/

mjkkirschner added 3 commits September 1, 2023 11:10

fix most memory leaks from opening and closing Dynamo Splash Screen (D…

38a0567

…ynamoDS#14344) * fixing mem leaks in progress * fix test and remove todo

Merge branch 'master' of https://github.com/DynamoDS/Dynamo

bd9071a

works and old graphs find method

06e24bf

mjkkirschner changed the title ~~Codeblockdisplay~~ Revert preview string transformtion and allow users to use numeric format when converting string to object. Sep 1, 2023

mjkkirschner changed the title ~~Revert preview string transformtion and allow users to use numeric format when converting string to object.~~ WIP Revert preview string transformtion and allow users to use numeric format when converting string to object. Sep 5, 2023

mjkkirschner added this to the 2.19.0 milestone Sep 5, 2023

mjkkirschner requested review from QilongTang, aparajit-pratap and sm6srw September 5, 2023 15:28

This comment was marked as outdated.

Sign in to view

mjkkirschner added 4 commits September 5, 2023 15:46

try to get tests passing

7a1a344

fix default values

bd25c84

add two new tests

remove duplicated builtin function endpoint

17de8d6

save status

6026229

mjkkirschner mentioned this pull request Sep 6, 2023

partially revert #13744 and add comments #14377

Merged

9 tasks

mjkkirschner added DNM Do not merge. For PRs. requires design labels Sep 6, 2023

QilongTang removed this from the 2.19.0 milestone Sep 6, 2023

avidit removed the requires design label Apr 24, 2024

merge conflict

22fcb33

mjkkirschner added 5 commits October 30, 2024 17:36

builtin method tests are passing

f981a14

Merge branch 'master' of https://github.com/DynamoDS/Dynamo into code…

8fe9ebe

…blockdisplay

update graph tests

dcc9d41

new builtin tests

45753f0

fix node doc tests

74a0b94

mjkkirschner removed the DNM Do not merge. For PRs. label Oct 31, 2024

mjkkirschner changed the title ~~WIP Revert preview string transformtion and allow users to use numeric format when converting string to object.~~ introduce new string from nodes that accept numeric format specifiers Oct 31, 2024

mjkkirschner added 3 commits November 5, 2024 12:04

update node names from review comments

f90f12e

docs

6d6f588

add experimental glyph on nodeview

2f0692f

QilongTang added this to the 3.5 milestone Nov 7, 2024

mjkkirschner added 4 commits November 7, 2024 12:00

this works to set experimental namespaces as experimetnal

4f7e142

add feature flag check for ui

81d0400

review comments

api

a877951

update tests

d11ed9f

conflict

0e73d61

aparajit-pratap reviewed Nov 8, 2024

View reviewed changes

doc/distrib/NodeHelpFiles/CoreNodeModels.FormattedStringFromArray.md Outdated Show resolved Hide resolved

aparajit-pratap reviewed Nov 8, 2024

View reviewed changes

doc/distrib/NodeHelpFiles/CoreNodeModels.FormattedStringFromArray.md Outdated Show resolved Hide resolved

aparajit-pratap reviewed Nov 8, 2024

View reviewed changes

aparajit-pratap approved these changes Nov 8, 2024

View reviewed changes

QilongTang reviewed Nov 11, 2024

View reviewed changes

QilongTang approved these changes Nov 11, 2024

View reviewed changes

review comments

e3d33e8

mjkkirschner merged commit 7ec4fc7 into DynamoDS:master Nov 11, 2024
21 of 23 checks passed

mjkkirschner deleted the codeblockdisplay branch November 11, 2024 21:57

avidit mentioned this pull request Nov 14, 2024

String to Integer conversion adds decimals to integer #14160

Closed

DYN-5965: introduce new string from nodes that accept numeric format specifiers #14369

DYN-5965: introduce new string from nodes that accept numeric format specifiers #14369

Uh oh!

Conversation

mjkkirschner commented Sep 1, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Purpose

Declarations

Release Notes

Reviewers

FYIs

Uh oh!

This comment was marked as outdated.

Uh oh!

This comment was marked as outdated.

This comment was marked as outdated.

github-actions bot commented Oct 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

UI Smoke Tests

Uh oh!

Amoursol commented Nov 8, 2024

Uh oh!

Amoursol commented Nov 8, 2024

Uh oh!

mjkkirschner commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mjkkirschner commented Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

aparajit-pratap Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

aparajit-pratap Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jingyi-Wen commented Nov 8, 2024

Uh oh!

aparajit-pratap Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Nov 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap Nov 8, 2024

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aparajit-pratap left a comment

Choose a reason for hiding this comment

Uh oh!

QilongTang Nov 11, 2024

Choose a reason for hiding this comment

Uh oh!

QilongTang Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner Nov 11, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mjkkirschner commented Nov 11, 2024

Uh oh!

Uh oh!

mjkkirschner commented Nov 11, 2024

mjkkirschner commented Sep 1, 2023 •

edited

Loading

github-actions bot commented Oct 29, 2024 •

edited

Loading

mjkkirschner commented Nov 8, 2024 •

edited

Loading

mjkkirschner commented Nov 8, 2024 •

edited

Loading

mjkkirschner Nov 11, 2024 •

edited

Loading

aparajit-pratap Nov 8, 2024 •

edited

Loading

aparajit-pratap Nov 8, 2024 •

edited

Loading

mjkkirschner Nov 11, 2024 •

edited

Loading

QilongTang Nov 11, 2024 •

edited

Loading

mjkkirschner Nov 11, 2024 •

edited

Loading