Skip to content

Drop List and IEnumerable conversions #963

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

Closed

Conversation

filmor
Copy link
Member

@filmor filmor commented Sep 26, 2019

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
Copy link
Member

lostmsu commented Sep 27, 2019

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
Copy link
Member Author

filmor commented Sep 27, 2019

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
Copy link
Member

lostmsu commented Sep 27, 2019

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

@filmor
Copy link
Member Author

filmor commented Sep 28, 2019

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
Copy link
Member

lostmsu commented Sep 29, 2019

@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
Copy link
Member Author

filmor commented Sep 30, 2019

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
Copy link
Member

lostmsu commented Sep 30, 2019

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

@filmor
Copy link
Member Author

filmor commented Oct 1, 2019

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
Copy link
Contributor

Cronan commented Oct 2, 2019

What's the rationale for this please?

@filmor
Copy link
Member Author

filmor commented Oct 2, 2019

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
Copy link
Contributor

Cronan commented Oct 2, 2019

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
Copy link
Contributor

Cronan commented Oct 8, 2019

Any chance of getting a look at those issues?

@koubaa
Copy link
Contributor

koubaa commented Jan 15, 2020

@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
Copy link

mmisol commented Jun 11, 2020

@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
Copy link
Member Author

filmor commented Jun 11, 2020

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

@lostmsu
Copy link
Member

lostmsu commented Jun 11, 2020

@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
Copy link
Member

lostmsu commented Dec 25, 2021

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