Use module context with iterables #1949
Conversation
| } | ||
|
|
||
| public static object? CallWithArgsTuple(object func, object?[] args, object argsTuple) { | ||
| public static object? CallWithArgsTuple(object func, object?[] args, IEnumerable argsTuple) { |
There was a problem hiding this comment.
I guess the goal with the signature change is to get rid of the context-less PythonOps.GetEnumerator call? Maybe adding the CodeContext argument would be "better"? Although looking at usage this function is basically useless and is a good candidate for removing/obsoleting...
There was a problem hiding this comment.
The goal was to get rid of the call to PythonOps.GetEnumerator here altogether. Arguably, a call with the context is "better" (in most cases but not all) than the context-less call, but in both cases it goes though PythonContext.TryConvertToIEnumerable, which unnecessarily "pollutes" the call site cache. It just caught my attention while reviewing the usage of PythonOps.GetEnumerator in the codebase.
The signature has changed to a more restrictive one, indeed, but looking at the usage and the name of of the method, one can assume that the type of parameter argsTuple is supposed to be a tuple… Indeed, originally I have changed the type to PythonTuple here, but since it is a public method, I thought that having IEnumerable will be less of an inconvenience for the user code that happens to use this method. For a while I also considered of typing it as ITuple, but (1) it would require adding this interface to PythonTuple (perhaps not a bad idea anyway for other reasons) and (2) no code in the wild makes calls to this method with Tuple because it is not enumerable, so why bother.
I have no opinion about obsolescence of this method; if you think it is worthwhile removing then go ahead!
2 participants
This PR builds further on #1721 and #1946 (issue #1718) by using
CodeContextwith various iterables.