Eliminate contextless PythonTuple constructor #1946
Conversation
|
|
||
| public PythonTuple([AllowNull]object o) { | ||
| _data = MakeItems(o); | ||
| public PythonTuple(CodeContext context, [AllowNull]object o) { |
There was a problem hiding this comment.
I guess this is technically a breaking change since the constructor was public. Doesn't seem unreasonable that someone could have been calling this from outside the codebase? Maybe we could keep it with the default context (we can add the Obsolete attribute).
There was a problem hiding this comment.
It is a breaking change, albeit a straightforward one: all is needed in user code is to add DefaultContext.Default as the first argument to have the same behaviour as before, although I was hoping that by making the parameter explicit it will prompt the user to some deliberation which context to use.
I don't think that adding another constructor with the signature of the old one will work — since they will only differ by CodeContext, the method resolver will not be able to pick one when dynamic code (i.e. Python code) is being run.
There was a problem hiding this comment.
If the overload is a conflict then go ahead and break it. 😊
Can't say I fully understand the whole concept of the context.
There was a problem hiding this comment.
Just to be sure, I ran a test with the old constructor added back, and sure enough, the resolver fails when trying to instantiate subclasses of tuple.
Perhaps I'll start a wiki page outlining all breaking changes like this one since 3.4.2 and the recommended workarounds, kind of what we have for upgrading from 2.x to 3.x. Hopefully it will be very short. In the release notes of 3.4.3 you could add a link to it.
There was a problem hiding this comment.
| object type = DynamicHelpers.GetPythonType(value); | ||
|
|
||
| object key = new PythonTuple(new[] { type, proto }); | ||
| object key = new PythonTuple(DefaultContext.Default, new[] { type, proto }); |
There was a problem hiding this comment.
I guess these are not PythonTuple.MakeTuple because IronPython.SQLite only has access to the public API surface?
There was a problem hiding this comment.
That's right. This is also the reason why in #1938 I directly included FakeInteropServices.cs. We could make this assembly a friend of IronPython.dll and gain access, but it is all-or-nothing internal access. I kind of like that IronPython.SQLite.cs has access only to the public API — it is a good test that the API is usable for programming.
Here, using DefaultContext.Default is not a problem because the type to enumerate is a CLR array. Just slightly less efficient.
2 participants
This further addresses Issue #1718, this time when enumerating
tuple. It builds on #1721, which handled the case oflist.As with
list, the solution is to specify the context in the constructor that uses enumeration to create the object. However, not always the current context is the best choice. For instance, when the object to enumerate is of one of the .NET types, the default context is a better solution. So alongside the constructor change, I have reviewed all instances of its use and decided whether the current or default context will be a better choice (in most practical cases). It is a heuristic, not a rule, so it is possible that in some scenarios my choices are not optimal (but still functionally correct).