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

Decode Python list into IList in function calls #10865

Merged
merged 9 commits into from
Jul 8, 2020

Conversation

mmisol
Copy link
Collaborator

@mmisol mmisol commented Jul 8, 2020

Purpose

Implements a decoder for Python sequence to transform them into generic
or non-generic IList. This relies on Python.NET 'PySequence.ToList'.

Also implements an encoder to perform a 'no-op'. This allows to treat
returned lists from function calls like .NET lists, which is the
behavior of Iron Python.

Declarations

Check these if you believe they are true

  • The codebase 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

@DynamoDS/dynamo

mmisol added 8 commits July 2, 2020 17:12
Adds tests for the following conversion scenarios:
- Python list => .NET IList (CPython fails)
- .NET array => Python list
- Python tuple => .NET array
- Python tuple => .NET IList (CPython fails)
- Python range => .NET array
- Python range => .NET IList (CPython fails)
- Python dict => .NET IDictionary (CPython fails)
- .NET IDictionary => Python dict (CPython unkonwn)

Also commented out some tests that fail in both engines but may be
considered to fix in CPython:
- .NET IList => Python list
- Python array => .NET IList
- Python dict => DS Dictionary

Other types considered but not added as tests:
- classes (can't convert them)
- dictionary view objects (uncommon)
- bytes, bytearray, memoryview (uncommon)
- set, frozenset (uncommon)
- complex (uncommon)
@mmisol mmisol requested a review from a team July 8, 2020 15:37
return false;
}

using (var pyList = PyList.AsList(pyObj))
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused by this using statement, does AsList return a .net list or a Python List?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a Python list

{
// This is a no-op to prevent Python.NET from encoding generic lists
// https://github.com/pythonnet/pythonnet/pull/963#issuecomment-642938541
return PyObject.FromManagedObject(value);
Copy link
Member

@mjkkirschner mjkkirschner Jul 8, 2020

Choose a reason for hiding this comment

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

does this break examples where I want to call a python function with a list of ints?

def sum(data):
    for i in data:

where data is a .net List<int> ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. Let me try it and come back with the answer.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It works. Do you want me to add it to the test?
Screen Shot 2020-07-08 at 12 39 42 PM

Copy link
Member

Choose a reason for hiding this comment

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

sure, nothing wrong with more tests?

Copy link
Member

Choose a reason for hiding this comment

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

okay, so l2 is a .net list because Flatten is a zt function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, it's List.Flatten from our core nodes

Copy link
Member

@mjkkirschner mjkkirschner left a comment

Choose a reason for hiding this comment

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

This looks good @mmisol - I would like to get the tests passing soon though - need to check with @alfredo-pozo on that.

@mmisol
Copy link
Collaborator Author

mmisol commented Jul 8, 2020

@mmisol mmisol merged commit 001662b into DynamoDS:master Jul 8, 2020
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.

2 participants