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 Dictionary Preview in Code block node #9020

Merged
merged 1 commit into from
Aug 2, 2018

Conversation

aparajit-pratap
Copy link
Contributor

@aparajit-pratap aparajit-pratap commented Jul 27, 2018

Purpose

The following node preview for a Dictionary in a CBN containing multple output ports does not expand currently. This is the screenshot after the fix:
image

This is because preferredDictionaryOrdering was a list of empty strings (3 in this case, one for each output port) and even though it should have been ignored it passed the check in DefaultWatchHandler::ProcessThing. This fix it to prevent the list from containing empty strings. Note that preferredDictionaryOrdering must only be populated for multi output nodes that have a MultiReturnAttribute.

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

@mjkkirschner

FYIs

@jnealb

@mjkkirschner
Copy link
Member

@aparajit-pratap just curious where did the extra entries come from? Does anything need to be updated similarly to this on the TS side?

@jnealb
Copy link
Collaborator

jnealb commented Jul 27, 2018

@aparajit-pratap @mjkkirschner Is there a JIRA task for this?

@aparajit-pratap
Copy link
Contributor Author

@jnealb
Copy link
Collaborator

jnealb commented Jul 28, 2018

@aparajit-pratap Thanks.

@aparajit-pratap
Copy link
Contributor Author

@mjkkirschner not sure what you mean by extra entries? Nothing needs to be updated on the TS side as this is more of a UI (preview) change in Dynamo.

@mjkkirschner
Copy link
Member

@aparajit-pratap misunderstood, so previously the dictionary was not displayed for multi return functions called in code blocks if one of the outputs was an empty string?

@aparajit-pratap
Copy link
Contributor Author

@mjkkirschner previously the dictionary could not be expanded in the preview if it was the output of a multi-output port code block node. Please don't confuse yourself with the contents of this Dictionary in the above example.

@aparajit-pratap
Copy link
Contributor Author

@mjkkirschner good to go?

@mjkkirschner
Copy link
Member

@aparajit-pratap LGTM

@aparajit-pratap aparajit-pratap merged commit 1842c74 into DynamoDS:master Aug 2, 2018
@aparajit-pratap aparajit-pratap deleted the fixDictPreview branch August 2, 2018 14:11
alfarok pushed a commit to alfarok/Dynamo that referenced this pull request Nov 8, 2018
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