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 unmarshaling of nested DS Dictionary #9023

Merged
merged 6 commits into from
Dec 17, 2018

Conversation

aparajit-pratap
Copy link
Contributor

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

Purpose

Fix issue with passing nested DesignScript.Builtin Dictionary to ZT node accepting generic Dictionary (Dictionary<string, object>) as argument.
JIRA link: https://jira.autodesk.com/browse/QNTM-4917

image

image

Zero touch node definitions used in this example:

 public static Dictionary<string, int> ReturnDictionary()
        {
            var dictionary = new Dictionary<string, int>();
            dictionary.Add("A", 1);
            dictionary.Add("B", 2);
            dictionary.Add("C", 3);
            dictionary.Add("D", 4);
            return dictionary;
        }
 [MultiReturn(new string[] {"H", "G", "F", "E"})]
        public static Dictionary<string, object> ReturnNestedDictionary()
        {
            var dict = ReturnDictionary();
            var dictionary = new Dictionary<string, object>();
            dictionary.Add("E", 1);
            dictionary.Add("F", 2);
            dictionary.Add("G", dict);
            dictionary.Add("H", false);
            return dictionary;
        }
 public static Dictionary<string, object> ReturnArbitraryDictionary()
        {
            var dict = ReturnDictionary();
            var dictionary = new Dictionary<string, object>();
            dictionary.Add("E", 1);
            dictionary.Add("F", new Dummy());
            dictionary.Add("G", dict);
            dictionary.Add("H", false);
            dictionary.Add("I", new object[] {1, 2, 3, 4, new DummyCollection()});

            return dictionary;
        }

        public static IDictionary ReturnIDictionary()
        {
            return ReturnDictionary();
        }

        public static IDictionary ReturnNestedIDictionary()
        {
            return ReturnNestedDictionary();
        }
public static IDictionary AcceptNestedDictionary(Dictionary<string, object> dictionary)
        {
            return dictionary;
        }
public static IDictionary AcceptIDictionary(IDictionary dictionary)
        {
            return dictionary;
        }

The following table shows the different combinations of Dictionary output (rows) being passed into node accepting Dictionary inputs (columns):

  Dictionary<string, object> IDictionary
Dictionary<string, object> Y Y
IDictionary Y Y
Nested Dictionary Y Y
Nested IDictionary Y Y
Dictionary w/ user-defined type Y Y
IDictionary w/ user-defined type Y Y
Arbitrary Dynamo Dictionary Y Y
Dictionary w/ List Y Y
Idictionary w/ List Y Y

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

@saintentropy

FYIs

@jnealb

@@ -533,7 +533,7 @@ private object ToIDictionary(StackValue dsObject, ProtoCore.Runtime.Context cont
{
try
{
targetDict[key] = Convert.ChangeType(d.ValueAtKey(key), valueType);
targetDict[key] = d.ValueAtKey(key);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see any necessity for the type conversion in this case. This conversion was throwing the error for the input object (DesignScript.Builtin.Dictionary) not being an IConvertible in the first place.

@aparajit-pratap aparajit-pratap changed the title Fix unmarshaling of nested DS Dictionary [DNM] Fix unmarshaling of nested DS Dictionary Jul 30, 2018
@aparajit-pratap aparajit-pratap added DNM Do not merge. For PRs. WIP labels Jul 30, 2018
@aparajit-pratap
Copy link
Contributor Author

Thanks @erfajo. Yes, that's one way to do it but we are also trying to fix the generic dictionary case.

@aparajit-pratap aparajit-pratap removed the DNM Do not merge. For PRs. label Dec 17, 2018
@aparajit-pratap aparajit-pratap changed the title [DNM] Fix unmarshaling of nested DS Dictionary Fix unmarshaling of nested DS Dictionary Dec 17, 2018
@aparajit-pratap aparajit-pratap added the PTAL Please Take A Look 👀 label Dec 17, 2018
@alfarok
Copy link
Contributor

alfarok commented Dec 17, 2018

@aparajit-pratap Changes look good to me. Just to confirm the previous behavior still functioned as expected but the node accepting the generic dictionary as an input displayed in a warning state?

Scratch that last question, I see what was going on
image

Let me know when this is ready for a final look, you mentioned you were adding some additional testing

@aparajit-pratap
Copy link
Contributor Author

Thanks @alfarok for the review and testing; I've added some more tests. Merging.

@aparajit-pratap aparajit-pratap merged commit 3e46380 into DynamoDS:master Dec 17, 2018
@aparajit-pratap aparajit-pratap deleted the nestedDict branch December 17, 2018 22:32
@aparajit-pratap aparajit-pratap mentioned this pull request Dec 19, 2018
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PTAL Please Take A Look 👀 WIP
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants