Fix type annotation in pandas.compat.numpy.function #25961
Conversation
Fix type errors as indicated by mypy + type sme other variables that cought my eye
|
@WillAyd I think this is my third open PR for typing. Should I wait for these to get reviewed before creating new issues and PR? |
|
Fine by me - thanks for all of the help so far!
…Sent from my iPhone
On Apr 2, 2019, at 7:29 AM, Vaibhav Vishal ***@***.***> wrote:
@WillAyd I think this is my third open PR for typing. Should I wait for these to get reviewed before creating new issues and PR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Ok, I will wait for at least one of them to get merged before working on new issues. |
Codecov Report
@@ Coverage Diff @@
## master #25961 +/- ##
==========================================
- Coverage 91.82% 91.81% -0.01%
==========================================
Files 175 175
Lines 52581 52582 +1
==========================================
- Hits 48280 48277 -3
- Misses 4301 4305 +4
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #25961 +/- ##
==========================================
- Coverage 91.98% 91.98% -0.01%
==========================================
Files 175 175
Lines 52375 52376 +1
==========================================
- Hits 48179 48176 -3
- Misses 4196 4200 +4
Continue to review full report at Codecov.
|
|
IMO, these type hints in this PR are a little over-fitted to this file. These default argument dictionaries are meant to map strings to anything that |
|
Yea was thinking the same thing, though I think maybe wrapping the containers in Final would be a good indication that these aren't meant to be modified: https://mypy.readthedocs.io/en/latest/final_attrs.html?highlight=immutable#details-of-using-final |
That's a fair proposition. |
|
My bad - fat fingers 😂 |
|
@vaibhavhrt : That looks good! If you could make similar modifications to your other changes, that would be great. |
|
@gfyoung think should convert dict to OrderedDict too? Not sure if a distinction between those is intentional so maybe make consistent? |
|
@WillAyd |
|
Well that's disappointing...not sure we want to add a third party library just yet. Was that not added until 3.7 |
runs without any error but at |
@WillAyd : |
|
@jreback do you have any thoughts on adding https://github.com/python/typing/tree/master/typing_extensions On the plus side maintained by Guido and could help smooth over compat issues leading up to 3.7 (where typing is no longer provisional). On the downside would be another dependency From there I think we could use Final, ChainMap, OrderedDict, Type, etc... Side note - I know we've used Type in a few places already but reading more up on it that wasn't added until 3.5.2 so I think that might have broken the code base for 3.5 and 3.5.1 |
|
not a big deal to add typing_extensions it’s pretty common now-a-days let’s do that in a separate patch |
|
Let me know when typing_extension has been added. @WillAyd if you want I can create an issue and PR to add typing_extensions to dependencies. But I am not sure how you are handling dependencies so so I might need some help. |
|
Sure give it a go! You should be able to see the PR to add Mypy which you can use for reference |
|
@vaibhavhrt can you merge master |
|
@jbrockmendel ok I will merge master. I have few more commits to push in this PR. But before I do that #25975 needs to be merged. The dependency |
|
@vaibhavhrt we won't be merging that PR mentioned #25961 (comment) as is, see the comments |
|
Yeah I see. If no one is already working on adding required types to
pandas.typing I can do that in a separate issue and PR.
…On Wed, 17 Apr 2019, 1:09 am Jeff Reback, ***@***.***> wrote:
@vaibhavhrt <https://github.com/vaibhavhrt> we won't be merging that PR
mentioned #25961 (comment)
<#25961 (comment)>
as is, see the comments
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#25961 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AViWLPKUBRThNMUvhH2OgPl3dyLXdrNaks5vhibsgaJpZM4cYL4Y>
.
|
|
Yes that’s correct.
…Sent from my iPhone
On Apr 23, 2019, at 10:44 PM, Vaibhav Vishal ***@***.***> wrote:
@vaibhavhrt commented on this pull request.
In pandas/compat/numpy/function.py:
> validate_round = CompatValidator(ROUND_DEFAULTS, fname='round',
method='both', max_fname_arg_count=1)
-SORT_DEFAULTS = OrderedDict()
+SORT_DEFAULTS = OrderedDict() # type: OrderedDict[str, Union[int, str, None]]
@WillAyd , Optional takes only one argument, so instead of
Union[X, Y, None]
Can I use optional like this 👇
Optional[Union[X, Y]]
I am a bit busy on weekdays, I will fix it if I get some time, otherwise it will be on saturday(friday for you), sorry about that.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
|
Hello @vaibhavhrt! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-04-25 06:44:26 UTC |
|
@WillAyd two tests are failing, but only thing I did was add comments, how can it make tests fail. |
|
@jreback I have fixed everything, you can review and merge it. |
|
thanks! |
git diff upstream/master -u -- "*.py" | flake8 --diff