Skip to content

Fix the crash when Autocompletion is triggered on output port of watch node.#11668

Merged
reddyashish merged 2 commits intoDynamoDS:masterfrom
reddyashish:master-1
May 6, 2021
Merged

Fix the crash when Autocompletion is triggered on output port of watch node.#11668
reddyashish merged 2 commits intoDynamoDS:masterfrom
reddyashish:master-1

Conversation

@reddyashish
Copy link
Collaborator

@reddyashish reddyashish commented May 5, 2021 •
edited
Loading

Purpose

This PR is to fix the crash that was happening when Autocompletion was triggered on output port of watch node. The reason for the crash was the output port type returned for the Watch node is something like "Node Output:var". When this was passed into the parser, it would throw an exception. So we will just be stripping that unnecessary string from the port type. Aabishkar has found that this was happening only for this particular node. I tried looking any other similar nodes but couldn't find any.

For now, we will be showing the default output suggestions for the Watch node.

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

Reviewers

@QilongTang @zeusongit @mjkkirschner

@josh2kand8

This comment was marked as spam.

Sign in to view
@reddyashish reddyashish changed the title Fix the crash when Autocompletion was triggered on output port of watch node. Fix the crash when Autocompletion is triggered on output port of watch node. May 5, 2021
@QilongTang
Copy link
Contributor

QilongTang commented May 5, 2021

hi @reddyashish , I think we can also try trim the output port type at https://github.com/DynamoDS/Dynamo/blob/master/src/Libraries/CoreNodeModels/Watch.cs#L19 to be just var so you dont have to trim it for every node again using these changes. Can you give that a try?

@reddyashish
Copy link
Collaborator Author

reddyashish commented May 5, 2021 •
edited
Loading

Yes that would be much better. @QilongTang Do you think changing the output port type of the watch node to just "var" cause any behavior change. I wasn't completely sure about it.

@QilongTang
Copy link
Contributor

QilongTang commented May 6, 2021

Yes that would be much better. @QilongTang Do you think changing the output port type of the watch node to just "var" cause any behavior change. I wasn't completely sure about it.

I think it should be fine. We can open a different PR for that and use self serve to validate

@reddyashish
Copy link
Collaborator Author

reddyashish commented May 6, 2021

I have made the changes in this PR only and running the self-serve. If anything fails, I will revert it.

@QilongTang
Copy link
Contributor

QilongTang commented May 6, 2021

I have made the changes in this PR only and running the self-serve. If anything fails, I will revert it.

Thanks, if everything pass, feel free to merge

@reddyashish
Copy link
Collaborator Author

reddyashish commented May 6, 2021

The self-serve validation has passed and tested it manually. Merging this.

@reddyashish reddyashish merged commit 3391d04 into DynamoDS:master May 6, 2021
@reddyashish reddyashish deleted the master-1 branch November 23, 2022 07:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@QilongTang QilongTang QilongTang approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@reddyashish @josh2kand8 @QilongTang