implement list codec #1084
implement list codec #1084
Conversation
|
Haven't done a full pass yet, but instead of doing "levels" you can just have 3 decoders, and register them in order from highest priority to lowest priority. E.g. Register(ListEncoder.Instance);
Register(CollectionDecoder.Instance);
Register(IterableDecoder.Instance);Also, please, do not dump all classes in the encoder file. |
|
@lostmsu I can change the physical layout but wanted to make sure I was on the right track first. I like the idea of having multiple codecs with priority, that way clients can choose which encoder they need. I'll work on that. |
src/embed_tests/Codecs.cs
Outdated
|
|
||
| //we'd have to copy into a list to do this. not the best idea to support it. | ||
| //maybe there can be a flag on listcodec to allow it. | ||
| Assert.IsFalse(codec.CanDecode(x, typeof(List<int>))); |
There was a problem hiding this comment.
@lostmsu With your idea of multiple codecs I can have a DeepCopyListCodec that takes any ienumerable and converts it by copying to a list. This is what we do right now in master and if we get rid of that implicit conversion the user can just register this codec if that causes an undesirable behavior change for them. @filmor what do you think?
There was a problem hiding this comment.
I am thinking, that Python.Runtime.dll should only contain codecs, that perform lossless conversions, perhaps even enabled by default. E.g. if a type is exactly builtins.list it is safe to convert it to IList<T> via some safe wrapper type PythonList<T>, that would contain a reference to the original object. Preferably, when this wrapper is passed back to Python it should either just pass the wrapped object, or behave exactly like a normal list would.
The codecs, that are not lossless, like converting a builtins.list to T[], and codecs, that act on relatively uncommon types like System.Uri should go to a separate extra package (created yesterday after seeing your PR): https://github.com/pythonnet/codecs
There was a problem hiding this comment.
@lostmsu done. I didn't go as far as to enable by default but that can be done as a followup PR. Please look closely at the test cases as I tried to capture the behavior of each of the three codecs to make sure it meets your expectation.
|
This is related to #912 |
|
@lostmsu While the unit tests work when I tried it from python I realized (again) that CanDecode is based on the type rather than the instance, so I changed that. Also I added some python tests |
|
If you're considering the performance improvement, this branch may help you. The
|
Thank you for sharing! I haven't measured performance issues but now I know what to try if I do. |
|
@lostmsu would you mind reviewing this again? I resolved all of your previous comments |
|
@koubaa sure, but can't do it until Saturday. |
|
@lostmsu I'll look to enable these by default (and handle issues associated w/that) in a later PR |
|
@lostmsu I rebased to fix the merge conflict. Is there anything else needed before this can be merged? |
|
@christabella I have been waiting for @lostmsu to merge it |
|
@lostmsu I rebased the changes to resolve the conflicts. Is there anything needed before this can be merged? |
|
@koubaa @christabella once the CI tests pass, I can take this. Note, however, that I have a different approach in mind, based on mixins and base class faking (a feature I have in my fork, that is yet to be upstreamed, its implementation is relatively involved). So this will probably be replaced before 3.0 is released. |
|
@lostmsu the CI is fixed |
What does this implement/fix? Explain your changes.
Implement conversions from python lists, iterables, and sequences to appropriate C# interfaces
...
Does this close any currently open issues?
Probably a few of them. I'll look for it later.
Any other comments?
I've done the codec for three levels of collections. roughly I map these:
The mapping isn't exactly perfect, naturally. But this is the best I think I can do right now.
I need to add some more codec unit tests and functional embedding tests.
Checklist
Check all those that are applicable and complete.
AUTHORSCHANGELOG