Skip to content

Conversation

@WillAyd
Copy link
Member

WillAyd commented Jun 26, 2019

Does this cause Mypy failures? Wasn’t think we’d add this as a setting as much as just fix the errors that using it would cause

@alimcmaster1
Copy link
Member Author

^ I'm just checking that it does cause failures. This way we can verify i've actually fixed the issues

@codecov
Copy link

codecov bot commented Jun 27, 2019

Codecov Report

Merging #27070 into master will decrease coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@ ## master #27070 +/- ## ========================================== - Coverage 92.04% 92.03% -0.01%  ========================================== Files 180 180 Lines 50714 50714 ========================================== - Hits 46680 46675 -5  - Misses 4034 4039 +5
Flag Coverage Δ
#multiple 90.67% <ø> (ø) ⬆️
#single 41.86% <ø> (-0.11%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 88.88% <0%> (-11.12%) ⬇️
pandas/core/frame.py 96.89% <0%> (-0.12%) ⬇️
pandas/util/testing.py 90.84% <0%> (-0.11%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d94146c...c098d1b. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 27, 2019

Codecov Report

Merging #27070 into master will increase coverage by 0.16%.
The diff coverage is n/a.

Impacted file tree graph

@@ Coverage Diff @@ ## master #27070 +/- ## ========================================== + Coverage 91.87% 92.03% +0.16%  ========================================== Files 180 180 Lines 50834 50714 -120 ========================================== - Hits 46703 46675 -28  + Misses 4131 4039 -92
Flag Coverage Δ
#multiple 90.67% <ø> (+0.16%) ⬆️
#single 41.86% <ø> (-0.12%) ⬇️
Impacted Files Coverage Δ
pandas/io/gbq.py 88.88% <0%> (-11.12%) ⬇️
pandas/io/excel/_openpyxl.py 84.71% <0%> (-3.23%) ⬇️
pandas/core/arrays/integer.py 96.3% <0%> (-1.32%) ⬇️
pandas/core/internals/construction.py 95.95% <0%> (-0.8%) ⬇️
pandas/core/dtypes/cast.py 90.72% <0%> (-0.34%) ⬇️
pandas/core/arrays/sparse.py 94.19% <0%> (-0.31%) ⬇️
pandas/core/config_init.py 95.8% <0%> (-0.28%) ⬇️
pandas/core/indexes/base.py 96.92% <0%> (-0.21%) ⬇️
pandas/core/arrays/timedeltas.py 88.82% <0%> (-0.2%) ⬇️
pandas/core/groupby/groupby.py 97.17% <0%> (-0.16%) ⬇️
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a173cac...04b49ed. Read the comment docs.

@WillAyd
Copy link
Member

WillAyd commented Jun 27, 2019

Gotcha. Just FYI you can also run mypy --new-semantic-analyzer pandas locally to see

@jreback jreback added the Typing type annotations, mypy/pyright type checking label Jun 27, 2019
@jreback jreback added this to the 0.25.0 milestone Jun 27, 2019
@alimcmaster1
Copy link
Member Author

first two errors removed, unsure of how to deal with the mypy error

pandas/io/json/__init__.py:5: error: Name 'json' is not defined 

Due to the fact we have: ( in our init.py)

del json, normalize, table_schema # noqa 

Any advice @WillAyd ?

@alimcmaster1 alimcmaster1 marked this pull request as ready for review June 30, 2019 16:03
@WillAyd
Copy link
Member

WillAyd commented Jun 30, 2019

Does converting the imports to abolute help at all? Seems like kind of a strange init file - maybe modeling it after pandas.io.excel would help and allow for removal of noqa

@pep8speaks
Copy link

pep8speaks commented Jun 30, 2019

Hello @alimcmaster1! 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-07-07 20:44:20 UTC
@WillAyd
Copy link
Member

WillAyd commented Jun 30, 2019

Just remove the noqa - shouldn't need them

@alimcmaster1
Copy link
Member Author

alimcmaster1 commented Jun 30, 2019

Latest test failures seem unrelated: ( currently looking )

pandas\tests\indexes\multi\test_format.py:14: _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ self = <contextlib._GeneratorContextManager object at 0x000002299F625860> type = None, value = None, traceback = None def __exit__(self, type, value, traceback): if type is None: try: > next(self.gen) E AssertionError: Did not see expected warning of class 'FutureWarning'. 
@WillAyd
Copy link
Member

WillAyd commented Jul 1, 2019

Code changes look good. Can you remove changes to mypy.ini and any requirements files?

@jreback any objection to renaming of files in json folder?

@jreback
Copy link
Contributor

jreback commented Jul 1, 2019

no objections

@alimcmaster1
Copy link
Member Author

Thanks for the review @WillAyd .

Just for my understanding what’s your motivation for removing the changes to mypy.ini it’s whats validating the mypy semantic analysis in CI - to avoid anyone introducing further errors?

Thanks

@WillAyd
Copy link
Member

WillAyd commented Jul 1, 2019

Just for my understanding what’s your motivation for removing the changes to mypy.ini it’s whats validating the mypy semantic analysis in CI - to avoid anyone introducing further errors?

Because we don't need to really pin a dependency here. Any of these changes are backwards compatible we are just preventing future mypy versions from breaking; no need to force a min version

read_json,
to_json,
json_normalize,
build_table_schema
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

those should be strings I think?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@jorisvandenbossche jorisvandenbossche changed the title Add semantic analyser Typing: Add mypy semantic analyser Jul 3, 2019
@jreback jreback removed this from the 0.25.0 milestone Jul 3, 2019
@WillAyd WillAyd changed the title Typing: Add mypy semantic analyser Typing: Support New Mypy Semantic Analyzer Jul 5, 2019
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm - over to you @jorisvandenbossche

@jreback
Copy link
Contributor

jreback commented Jul 5, 2019

@alimcmaster1 isn't the commit adding the semantic analyzer removed? IOW this doesn't look like it is triggering it.

@WillAyd
Copy link
Member

WillAyd commented Jul 5, 2019

I asked to remove the semantic analyzer from the config because it naturally becomes the default in the next Mypy release. Figured no need to set then and not setting maintains better backwards compat

@jreback
Copy link
Contributor

jreback commented Jul 5, 2019

I asked to remove the semantic analyzer from the config because it naturally becomes the default in the next Mypy release. Figured no need to set then and not setting maintains better backwards compat

ok, then we should set a min on mypy (when this comes out)? or at least on our CI do that?

@WillAyd
Copy link
Member

WillAyd commented Jul 5, 2019 via email

@jreback jreback added this to the 0.25.0 milestone Jul 5, 2019
@alimcmaster1
Copy link
Member Author

Updated as per your comments and green @jreback

@jreback jreback merged commit 845d4b8 into pandas-dev:master Jul 8, 2019
@jreback
Copy link
Contributor

jreback commented Jul 8, 2019

thanks @alimcmaster1

@alimcmaster1 alimcmaster1 deleted the mcmali-mypy branch December 25, 2019 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Typing type annotations, mypy/pyright type checking

5 participants