-
Notifications
You must be signed in to change notification settings - Fork 635
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
Marshall out iterables as lists in CPython #10894
Conversation
Adds support for marshalling out iterables that are not lists or dictionaries using the CPython engine. In order to achieve this, the check PyIter.IsIterable was used, along with some extra considerations, mentioned in the source with comments.
@@ -195,5 +195,34 @@ class myobj: | |||
Assert.AreEqual("Output could not be converted to a .NET value", exc.Message); | |||
} | |||
} | |||
|
|||
[Test] | |||
public void NonListIterablesCanBeOutput() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this test hit the condition on line 204
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not this test. One which failed without that part is this one https://github.com/DynamoDS/Dynamo/blob/master/test/Libraries/DynamoPythonTests/PythonEvalTestsWithLibraries.cs#L189
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so it's currently failing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, sorry if I didn't say it right. That test does work and hits that line.
What I meant is that at some point during my development of this I noticed the test failed, which led me to add the block that spans from line 201 to 205.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Tests passed here: https://master-15.jenkins.autodesk.com/view/DYN/job/DYN-DevCI_Self_Service/707/. Merging |
Purpose
Adds support for marshalling out iterables that are not lists or
dictionaries using the CPython engine. In order to achieve this, the
check PyIter.IsIterable was used, along with some extra considerations,
mentioned in the source with comments.
Declarations
Check these if you believe they are true
*.resx
filesReviewers
@mjkkirschner
FYIs
@DynamoDS/dynamo