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

Convert Python iterable to IList and IEnumerable #10879

Merged
merged 3 commits into from
Jul 10, 2020

Conversation

mmisol
Copy link
Collaborator

@mmisol mmisol commented Jul 10, 2020

Purpose

Adds support to convert any Python iterable to an IList or IEnumerable
in the context of a function call using the CPython engine. The decoder
now checks if the PyObject returned is iterable instead of a sequence,
which is supported by the function we are using in Python.NET. Also,
IEnumerable, both generic and non-generic, is declared as a target type
to allow more conversion scenarios.

This should now allow converting dictionary view types, set and
frozenset to IList. It should also allow these and any other types that
are iterable (list, tuple, dictionary) to be converted to IEnumerable.

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

@mjkkirschner

FYIs

@DynamoDS/dynamo

mmisol added 3 commits July 9, 2020 12:26
Adds support to convert any Python iterable to a list in the context of
a function call using the CPython engine. The support is added by
relaxing the check in the custom decoder to allow any iterable to come
through, not only sequences.

This should now allow converting dictionary view types, set and
frozenset.
@mmisol mmisol requested a review from mjkkirschner July 10, 2020 16:00
@@ -66,21 +68,29 @@ import clr

l3 = [[1,2],[3,4]]
# Python list (nested) => .NET IList<IList<>>
flatennedList = List.Flatten(l3)
flattenedList = List.Flatten(l3)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch

{ "one", 1 }, { "two", 2 }, { "three", 3 }, { "four", 4 }
};
var expectedKeys = new List<string> { "one", "three", "two" };
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it because List unordered so sequence does not matter here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The order is alphabetic


dk = List.AddItemToEnd('four', d.keys())
dv = List.AddItemToEnd(4, d.values())
di = List.AddItemToEnd(('four', 4), d.items())
Copy link
Contributor

Choose a reason for hiding this comment

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

Did we get all this supported by simply adding the IsIterable?

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. They were all iterable, but we were checking if they were sequences, which they are not. Since our Python.NET code supported all iterables we are good!

from DSCore import List

s = { 'hello' }
fs = frozenset(s)
Copy link
Contributor

Choose a reason for hiding this comment

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

What is a frozenset?

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 like an immutable set.

@mmisol
Copy link
Collaborator Author

mmisol commented Jul 10, 2020

Got the usual 23 failures related to the visualization tests. Should this be good to go @QilongTang ?

@QilongTang
Copy link
Contributor

@mmisol Sure go for it

@mmisol mmisol merged commit 11f953b into DynamoDS:master Jul 10, 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.

3 participants