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

Marshall out iterables as lists in CPython #10894

Merged
merged 3 commits into from
Jul 15, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 22 additions & 11 deletions src/Libraries/DSCPython/CPythonEvaluator.cs
Original file line number Diff line number Diff line change
Expand Up @@ -195,18 +195,15 @@ public static DataMarshaler OutputMarshaler
outputMarshaler.RegisterMarshaler(
delegate (PyObject pyObj)
{
if (PyList.IsListType(pyObj))
// First, check if we are dealing with a wrapped .NET object.
// This simplifies the cases that come afterwards, as wrapped
// .NET collections pass some Python checks but not others.
var clrObj = pyObj.GetManagedObject();
if (clrObj != null)
{
using (var pyList = new PyList(pyObj))
{
var list = new List<object>();
foreach (PyObject item in pyList)
{
list.Add(outputMarshaler.Marshal(item));
}
return list;
}
return outputMarshaler.Marshal(clrObj);
}
// Dictionaries are iterable, so they should come first
if (PyDict.IsDictType(pyObj))
{
using (var pyDict = new PyDict(pyObj))
Expand All @@ -222,6 +219,20 @@ public static DataMarshaler OutputMarshaler
return dict;
}
}
// Other iterables should become lists, except for strings
if (PyIter.IsIterable(pyObj) && !PyString.IsStringType(pyObj))
{
using (var pyList = PyList.AsList(pyObj))
{
var list = new List<object>();
foreach (PyObject item in pyList)
{
list.Add(outputMarshaler.Marshal(item));
}
return list;
}
}
// Special case for big long values: decode them as BigInteger
if (PyLong.IsLongType(pyObj))
{
using (var pyLong = PyLong.AsLong(pyObj))
Expand All @@ -236,7 +247,7 @@ public static DataMarshaler OutputMarshaler
}
}
}

// Default handling for other Python objects
var unmarshalled = pyObj.AsManagedObject(typeof(object));
if (unmarshalled is PyObject)
{
Expand Down
29 changes: 29 additions & 0 deletions test/Libraries/DynamoPythonTests/PythonEvalTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -195,5 +195,34 @@ class myobj:
Assert.AreEqual("Output could not be converted to a .NET value", exc.Message);
}
}

[Test]
public void NonListIterablesCanBeOutput()
Copy link
Member

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?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Copy link
Member

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?

Copy link
Collaborator Author

@mmisol mmisol Jul 15, 2020

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.

{
var code = @"
s = { 'hello' }
fs = frozenset({ 'world' })
d = { 'one': 1 }
dk = d.keys()
dv = d.values()
di = d.items()

OUT = s,fs,dk,dv,di
";
var expected = new ArrayList
{
new ArrayList { "hello" },
new ArrayList { "world" },
new ArrayList { "one" },
new ArrayList { 1 },
new ArrayList { new ArrayList { "one", 1 } }
};
var empty = new ArrayList();
foreach (var pythonEvaluator in Evaluators)
{
var output = pythonEvaluator(code, empty, empty);
Assert.AreEqual(expected, output);
}
}
}
}