Skip to content

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

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