Some improvements to type hints #230
Conversation
These come from comments on the NumPy pull request numpy/numpy#18585.
| @@ -19,7 +19,7 @@ Joins a sequence of arrays along an existing axis. | |||
|
|
|||
| #### Parameters | |||
|
|
|||
| - **arrays**: _Tuple\[ <array>, ... ]_ | |||
| - **arrays**: _Union\[Tuple\[ <array>, ... ], List\[ <array> ] ]_ | |||
There was a problem hiding this comment.
Would it make more sense to use Sequence, rather than a Union?
There was a problem hiding this comment.
That was mentioned here numpy/numpy#18585 (comment). If we agree Sequence is better we can use that instead.
There was a problem hiding this comment.
I am fairly happy with requiring a tuple or list. Sequence is quite broad (e.g. is an array a sequence?), and I'm not that interested in supporting such flexibility for custom containers, it ends up hurting more than helping often.
| @@ -154,7 +154,7 @@ Joins a sequence of arrays along a new axis. | |||
|
|
|||
| #### Parameters | |||
|
|
|||
| - **arrays**: _Tuple\[ <array>, ... ]_ | |||
| - **arrays**: _Union\[Tuple\[ <array>, ... ], List\[ <array> ] ]_ | |||
There was a problem hiding this comment.
Same comment as above regarding Sequence.
| @@ -57,7 +57,7 @@ Convert the input to an array. | |||
|
|
|||
| #### Parameters | |||
|
|
|||
| - **obj**: _Union\[ float, NestedSequence\[ bool | int | float ], SupportsDLPack, SupportsBufferProtocol ]_ | |||
| - **obj**: _Union\[ <array>, float, NestedSequence\[ bool | int | float ], SupportsDLPack, SupportsBufferProtocol ]_ | |||
There was a problem hiding this comment.
I can raise an issue/PR if more appropriate, but while you're at it—as the docs say obj can be a Python scalar, am I correct in saying that bool and int should be part of the top-level Union type hint alongside float?
There was a problem hiding this comment.
Technically bool and int are already covered by float (they sub-types), so the main rule we have used for determining whether or not to add bool and/or int explicitly is: does it help for clarity?
In this case the output dtype is different for bool or int scalar input, so makes sense to add explicitly.
|
Can this be merged? I'm not clear if we should be using Sequence or Union[Tuple,List]. |
Let's get this in since it's been pending for a while. If there is follow-up discussion on sequence vs list/tuple we can deal with it separately. Thanks @asmeurer, all. |
And other small fixes. The type hint fixes come from numpy/numpy#18585, except for the
Literalchange in qr which comes from me.One thing that I didn't address here is the type for the
streamargument in__dlpack__. See this discussion numpy/numpy#18585 (comment).