Type args and kwargs in pipe
#823
Merged
Conversation
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
Contributor
Author
|
Started out typing out frame's tests/test_frame.py:1393: error: Expression is of type <nothing>, not "DataFrame" [assert-type]
tests/test_frame.py:1394: error: Argument 1 to "pipe" of "DataFrame" has incompatible type "Callable[[DataFrame, int, str], DataFrame]"; expected "Callable[[DataFrame, int, str], <nothing>] | tuple[Callable[..., <nothing>], str]" [arg-type]Looking into this! |
twoertwein
reviewed
Dec 7, 2023
Dr-Irv
requested changes
Dec 11, 2023
Collaborator
Dr-Irv
left a comment
Dr-Irv
left a comment
There was a problem hiding this comment.
There is an issue here with respect to having pipe defined in core/frame.pyi and core/series.pyi . I think if you took the concept proposed in this PR and used it in generic.pyi and removed it from core/frame.pyi, things would work for both Series and DataFrame.
So can you make that change, and then have tests for both Series.pipe() and DataFrame.pipe() ?
Contributor
Author
Got it, that sounds good to me! |
Contributor
Author
|
Just to try things out, implemented the more complicated function with positional-only args, keyword-only args, etc. This fails in current tests. tests/test_frame.py:1401: error: Expression is of type <nothing>, not "DataFrame" [assert-type]
tests/test_frame.py:1403: error: Argument 1 to "pipe" of "DataFrame" has incompatible type "Callable[[DataFrame, int, list[float], str, NamedArg(tuple[int, int], 'keyword_only')], DataFrame]"; expected "Callable[[DataFrame, int, list[float], str, tuple[int, int]], <nothing>] | tuple[Callable[..., <nothing>], str]" [arg-type]
Found 2 errors in 1 file (checked 224 source files)However, when I upgrade mypy to latest version (1.7.1), the test does pass. However, it also raises a bunch of new errors elsewhere in code, I think all of them are of now unused mypy 1.7.1 errorsSo in order to implement this I'm thinking we need to:
Happy to attempt this in another PR first if this is something we are interested in. Trying this out still in the |
Member
|
Main uses mypy 1.7.1 now, so you can just rebase/merge and all the mypy warnings should disappear. |
paw-lu
force-pushed
the
pipe-typing
branch
3 times, most recently
from
07fc875 to
4fb7876
Compare
Contributor
Author
|
Cool, rebased on top of the latest changes and things mostly seem to pass. Added a lot of tests for I have some local failures right now on: 1. pytest2. pyrightI think they're unrelated to these changes, but I'll switch back to main to check if they pass locally on my machine later before proceeding with the rest of the changes. |
paw-lu
force-pushed
the
pipe-typing
branch
2 times, most recently
from
15bbd54 to
19a230b
Compare
Contributor
Author
|
Ok, seems that test that are current failing on pyright and pytest were failing in the commit prior to mine, so I'm going to ignore those. I've completed tests for frame and implementation. I plan to just do one more thing from here: add tests for pipe to |
Dr-Irv
requested changes
Dec 22, 2023
Collaborator
Dr-Irv
left a comment
Dr-Irv
left a comment
There was a problem hiding this comment.
This all looks pretty good. Can you add tests in test_series.py as well similar to what you have for test_frame.py ?
Contributor
Author
|
Added tests in Now that it seems like will make |
Collaborator
Yes, so make the change in your PR to use the overloads (as in the example at microsoft/pyright#6796 (comment) ), and you might get a failure in the tests if you don't include an We probably will have to wait until 12/26 or later to get this all done. |
paw-lu
force-pushed
the
pipe-typing
branch
2 times, most recently
from
f4807a9 to
abc5748
Compare
Dr-Irv
requested changes
Dec 26, 2023
Dr-Irv
approved these changes
Dec 27, 2023
This was referenced Jan 5, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
GroupBy.pipe()missing types in arguments and correct return type #738assert_type()to assert the type of any return valueFollow up to pandas-dev/pandas#56359. Type out
argsandkwargsinpipemethods, and make all instances of thepipemethod consistent in their typing ( #738).