-
-
Notifications
You must be signed in to change notification settings - Fork 18.5k
TYP overload fillna #40737 #40887
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
Merged
Merged
TYP overload fillna #40737 #40887
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
79f6b50
TYP: Added overloads for fillna() in frame.py and series.py
9b6966a
TYP: Added overloads for fillna() in frame.py and series.py #40737
fce2d11
TYP: Added fillna() overloads to generic.py #40727
bd93f31
Merge branch 'typ-fillna' of github.com:LarWong/pandas into typ-fillna
16b42e7
TYP: removed generic overloads #40737
2ab41f8
fixed redundant cast error
d6cc081
Merge remote-tracking branch 'upstream/master' into typ-fillna
a641df2
reverting prior changes
45cc60c
remove cast again
1c33bd5
Merge remote-tracking branch 'upstream/master' into typ-fillna
b6a4fac
removed unnecessary overloads in frame.py and series.py
2adcaf7
fixed overloads
4b7744e
Merge remote-tracking branch 'upstream/master' into typ-fillna
0c3d28d
reverted value typing
1c8dd1a
remove extra types (lets keep this to overloads)
MarcoGorelli File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
can we not put these in .pyi?
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 was told to add them directly to these files since existing overloads were already there
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.
@jreback yes, that would be great! Thing is though, the
.pyi
files require you to define all methods of a moduleGiven the sheer number of methods this module has, I'd suggest taking this PR with the overloads here, and then moving the overloads (along with annotations for all other methods) to a
pandas/core/frame.pyi
fileThere 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.
@LarWong I'll get back to you on typing
value
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 we want to go the route of using stubs for python files
not that I think it should be done here, but it is possible to partially type a module using stubs.
https://github.com/python/typeshed/blob/master/CONTRIBUTING.md#incomplete-stubs
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.
Thanks @simonjayhawkins , didn't know that was possible. Why not use partial stubs for overloads? Because some methods, like
Series.drop
pandas/pandas/core/series.py
Lines 4478 to 4487 in 84d9c5e
have 5 (five!) arguments with defaults before
inplace
, leading to...34 overloads 🤯 ! And even if we disallowedlabels
being passed asNone
in that one, that would still leave us with 18 overloads!For typing
value
here, do you thinkScalar | Mapping[Hashable, Scalar] | Series | DataFrame | None
would be correct?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.
The problem is that if a stub file is present it takes precedence over the python file. so we cannot ensure internal consistency and need to duplicate the type annotations to be able to check the functions in the module itself.
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 think we are planning to to drop the inplace argument, hopefully pandas 2.0 and we won't need all these overloads. #16529