TYP: use overload to refine return type of reset_index #33519
Conversation
| self, | ||
| level: Optional[Union[Hashable, Sequence[Hashable]]] = None, | ||
| drop: bool = False, | ||
| *, |
There was a problem hiding this comment.
Is it possible to combine Literal and overload here instead of this? I don’t think this is 100% accurate if a user provides inplace=False
There was a problem hiding this comment.
we can't use Literal til Python 3.8 or unless we adopt typing extensions.
If we pass inplace=False the return type will match the second signature and so the return type will be the Union. This is not inaccurate and no change from the status quo.
If we don't pass an inplace argument, the return type will be DataFrame and additional asserts, casts or ignores aren't needed. This is an improvement from the status quo.
There was a problem hiding this comment.
Whether explicitly passing inplace=False or just sticking with the default signature the expected return type shouldn't be different, which is what is being introduced here. The ideal overload should be something like "return None if inplace=True, otherwise return a DataFrame".
While the current approach may appease most type checking it introduces unexpected behavior when providing the inplace keyword; would rather try to integrate a Literal type or actually deprecate inplace.
| self, | ||
| level: Optional[Union[Hashable, Sequence[Hashable]]] = None, | ||
| drop: bool = False, | ||
| *, |
There was a problem hiding this comment.
Whether explicitly passing inplace=False or just sticking with the default signature the expected return type shouldn't be different, which is what is being introduced here. The ideal overload should be something like "return None if inplace=True, otherwise return a DataFrame".
While the current approach may appease most type checking it introduces unexpected behavior when providing the inplace keyword; would rather try to integrate a Literal type or actually deprecate inplace.
can't use Literal yet.
can't do this till 2.0 closing to clear queue as requested changes not actionable. can potentially reopen depending on outcome of #33274 |
|
@simonjayhawkins @WillAyd is there a consensus on how to move forward here? |
|
So I wouldn't object to vendoring typing_extensions and doing this with Literal values - @simonjayhawkins do you have any particular thoughts on that? As is though I think this creates as many nuances as it solves, so not a huge fan of going forward in current state |
No description provided.