Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DYN-6893 input symbol nodes should have default names - pt 1 #15193

Merged
merged 4 commits into from
May 7, 2024

Conversation

mjkkirschner
Copy link
Member

@mjkkirschner mjkkirschner commented May 6, 2024

Purpose

When a user creates a input node (also known as a symbol node) in a custom node workspace previously the input node was empty. If left in this state and saved, the input symbol would be invalid as it did not have a default name, and would raise a warning and disallow save when next opened.

Just for the curious the input does have a default type though (var[]..[]).

To address this issue - now when an Input node is constructed now we set the name to a default (I'm just using input for now, which is what we use as a display name when the name is empty)
We now also show the type hint in default string to make the syntax more discoverable, as well as a comment - we could also show the default value syntax easily enough but I thought that might be info overload, as well as potentially having unintended consequences.

before:
Screenshot 2024-05-06 at 10 58 46 AM

after:
Screenshot 2024-05-07 at 1 20 46 PM
Screenshot 2024-05-07 at 1 20 40 PM

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

By default creating inputs in a custom node will not put the custom node into an invalid state.

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

@Jingyi-Wen thoughts on this approach or what the default name / default string should be?

Copy link

github-actions bot commented May 6, 2024

UI Smoke Tests

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

@mjkkirschner
Copy link
Member Author


InputSymbol = String.Empty;
//TODO localize
InputSymbol = new TypedParameter($"DefaultInputName", "var",-1,null).ToCommentNameString();
Copy link
Contributor

Choose a reason for hiding this comment

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

So by default all input nodes will have the same name ?
so by default all input names of a custom node will have the same name. Do you think it might be worth create a unique name ?...maybe not
Also do output nodes not have the same issue ?

Copy link
Member Author

@mjkkirschner mjkkirschner May 6, 2024

Choose a reason for hiding this comment

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

So by default all input nodes will have the same name ? - yes

so by default all input names of a custom node will have the same name - yes - but note that there should not be inputNodes in home workspaces.

yes, by default they will have the same name, I considered using a part of the node guid so there's a very high chance the name would be unique - ie InputName_12345 or something like that.

Something to consider is that today, they already have the same name, it's just "".

Output nodes do not have any validation as far as I can tell.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

See the ticket for this pull request: https://jira.autodesk.com/browse/DYN-6893

@mjkkirschner mjkkirschner merged commit b120519 into DynamoDS:master May 7, 2024
19 checks passed
@mjkkirschner mjkkirschner deleted the dyn6893 branch May 7, 2024 22:14
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.

2 participants