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

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

Merged
merged 2 commits into from
May 6, 2021

Conversation

reddyashish
Copy link
Contributor

@reddyashish reddyashish commented May 5, 2021

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.

@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

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
Contributor Author

reddyashish commented May 5, 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.

@QilongTang
Copy link
Contributor

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
Contributor Author

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

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
Contributor Author

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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants