Skip to content

Drop List and IEnumerable conversions#963

Closed
filmor wants to merge 4 commits into
pythonnet:masterfrom
filmor:disable-implicit-list-conversion
Closed

Drop List and IEnumerable conversions#963
filmor wants to merge 4 commits into
pythonnet:masterfrom
filmor:disable-implicit-list-conversion

Conversation

@filmor

@filmor filmor commented Sep 26, 2019

Copy link
Copy Markdown
Member

What does this implement/fix? Explain your changes.

Drop implicit conversion of List<> and IEnumerable.

Does this close any currently open issues?

Heaps, I'll list them later

Any other comments?

Breaks the interface, so we'll have to announce this loudly and raise the minor version number.

Checklist

Check all those that are applicable and complete.

  • Make sure to include one or more tests for your change
  • If an enhancement PR, please create docs and at best an example
  • Add yourself to AUTHORS
  • Updated the CHANGELOG

@filmor filmor force-pushed the disable-implicit-list-conversion branch 2 times, most recently from 6e202f6 to f1df348 Compare September 27, 2019 06:14
@filmor filmor force-pushed the disable-implicit-list-conversion branch from f1df348 to ea8df83 Compare September 27, 2019 06:26
@lostmsu

lostmsu commented Sep 27, 2019

Copy link
Copy Markdown
Member

Just want to confirm if we are to bump up major version with it. Seems like a non-trivial change, that might break a lot of things, that are working now.

It is not immediately obvious from this change, but if I used to pass an array to a parameter, that expects Python list, would this no longer work anymore?

@filmor

filmor commented Sep 27, 2019

Copy link
Copy Markdown
Member Author

I'm trying to keep the amount of changes in behaviour to a minimum. I'm not going to bump the major version (i.e. this will not be 3.0) but instead the minor version 2.5 as I've had enough of the "lists behave weirdly" kind of issues we are seeing since this was introduced.

.NET array objects are still IEnumerable<> and thus will keep being iterable, so for Python functions that don't (wrongly) check the type explicitly this will still work. We can as a further step also implement the Python Sequence Protocol for ICollection<>, but one step after another :)

@lostmsu

lostmsu commented Sep 27, 2019

Copy link
Copy Markdown
Member

@filmor what about functions, that do len(arg)?

@filmor

filmor commented Sep 28, 2019

Copy link
Copy Markdown
Member Author

That is the second paragraph of my comment ;)

IEnumerable doesn't allow you to do an efficient len either, that's what ICollection<> is for and we would have to implement at least sq_len then.

@lostmsu

lostmsu commented Sep 29, 2019

Copy link
Copy Markdown
Member

@filmor I meant that previously when you pass a List<T> to a Python function, that uses len(lst) it would work, but if List<T> is no longer converted to list, that will stop working.

@filmor

filmor commented Sep 30, 2019

Copy link
Copy Markdown
Member Author

List<T> implements ICollection<T> which has a Count property which should in turn be used to implement the sq_length slot of the sequence protocol (https://docs.python.org/3/c-api/typeobj.html#sequence-object-structures)

@lostmsu

lostmsu commented Sep 30, 2019

Copy link
Copy Markdown
Member

@filmor "should be used" but not "already used". Are you planning to implement sq_length too before the next release?

@filmor

filmor commented Oct 1, 2019

Copy link
Copy Markdown
Member Author

This PR is a draft, I plan to work on it until all tests pass. If that means implementing sq_length, then that's what I am going to do.

@Cronan

Cronan commented Oct 2, 2019

Copy link
Copy Markdown
Contributor

What's the rationale for this please?

@filmor

filmor commented Oct 2, 2019

Copy link
Copy Markdown
Member Author

The rationale is the huge number of issues related to the automatic conversion. Just being able to use a .NET IEnumerable as an iterable and ICollection as a sequence from Python catches most use cases of the conversion without breaking code like l = obj.GetCSharpList(); obj.CallWithCSharpList(l).

@Cronan

Cronan commented Oct 2, 2019

Copy link
Copy Markdown
Contributor

The rationale is the huge number of issues related to the automatic conversion. Just being able to use a .NET IEnumerable as an iterable and ICollection as a sequence from Python catches most use cases of the conversion without breaking code like l = obj.GetCSharpList(); obj.CallWithCSharpList(l).

OK, thanks. It would be nice to close down a bunch of issues.

@Cronan

Cronan commented Oct 8, 2019

Copy link
Copy Markdown
Contributor

Any chance of getting a look at those issues?

@koubaa

koubaa commented Jan 15, 2020

Copy link
Copy Markdown
Contributor

@filmor @lostmsu Even if we drop it if we provide a codec (see 1022) that is not registered by default users can easily opt back into the current behavior by registering it themselves.

@mmisol

mmisol commented Jun 11, 2020

Copy link
Copy Markdown

@filmor First of all, thanks for providing such a great library!

Do you think this can go in as part of the 2.5 release? I see it's not in the RC, but would love to see this fixed.

@filmor

filmor commented Jun 11, 2020

Copy link
Copy Markdown
Member Author

@mmisol Sorry, 2.5 is a minor release, we can't break the API even though it would be an improvement.

@lostmsu

lostmsu commented Jun 11, 2020

Copy link
Copy Markdown
Member

@mmisol injecting a no-op codec will allow you to override this behavior with 2.5: https://github.com/pythonnet/pythonnet/blob/master/src/runtime/Codecs/RawProxyEncoder.cs

E.g. (might contain errors)

class RawListEncoder: RawProxyEncoder {
  public override bool CanEncode(Type type)
    => type.IsGenericType && type.GetGenericTypeDefinition() == typeof(List<>);
}

PyObjectConversions.RegisterEncoder(new RawListEncoder());

See also an example from Python side: f707698#diff-f8910e440ff4d17505744bd4c7016697R706

@lostmsu

lostmsu commented Dec 25, 2021

Copy link
Copy Markdown
Member

I think this has been superseded in 3.0 branch. These default conversions are no longer enabled but can be reimplemented with codecs.

@lostmsu lostmsu closed this Dec 25, 2021
@filmor filmor deleted the disable-implicit-list-conversion branch December 25, 2021 22:49
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.

5 participants