-
-
Notifications
You must be signed in to change notification settings - Fork 18.4k
CLN: follow-ups #29600
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
CLN: follow-ups #29600
Conversation
pandas/core/reshape/merge.py
Outdated
_right = _validate_operand(right) | ||
self.left = self.orig_left = _validate_operand(_left) # type: "DataFrame" | ||
self.right = self.orig_right = _validate_operand(_right) # type: "DataFrame" | ||
_left: "DataFrame" = _validate_operand(left) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
since _validate_operand
is typed (with a return type), the variable annotation is redundant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we dont want it just for the reader?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i don't think so. doing this sort of thing could loosen type checking, say if for instance _validate_operand
returned a subclass of DataFrame
this would pass mypy and the variable would no longer have the type of the subclass
import pandas as pd
class MyDataFrame(pd.DataFrame):
pass
def some_function() -> MyDataFrame:
return MyDataFrame()
loose: pd.DataFrame = some_function()
reveal_type(loose)
tight = some_function()
reveal_type(tight)
$ mypy test.py
test.py:13: note: Revealed type is 'pandas.core.frame.DataFrame'
test.py:15: note: Revealed type is 'test.MyDataFrame'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
got it, thanks. will update
thanks @jbrockmendel |
includes requested followups for reshape.merge cc @simonjayhawkins