-
-
Notifications
You must be signed in to change notification settings - Fork 18.1k
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
TYP: Assorted_Typing_Fix in pandas.io #25939
TYP: Assorted_Typing_Fix in pandas.io #25939
Conversation
There were some unused imports in packers module - i've left them in for now, not sure if the linter will complain as it was throwing warnings in flake8 |
Codecov Report
@@ Coverage Diff @@
## master #25939 +/- ##
==========================================
- Coverage 91.81% 41.92% -49.9%
==========================================
Files 175 175
Lines 52580 52597 +17
==========================================
- Hits 48278 22049 -26229
- Misses 4302 30548 +26246
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #25939 +/- ##
==========================================
- Coverage 91.9% 91.89% -0.01%
==========================================
Files 175 175
Lines 52485 52485
==========================================
- Hits 48235 48230 -5
- Misses 4250 4255 +5
Continue to review full report at Codecov.
|
@WillAyd Was getting an azure pipeline error when removing ununsed import |
@ryankarlos that does seem strange - have you been able to debug that issue locally? |
@WillAyd Something strange - when i debug locally I can reproduce the same error - problem seems to be mainly with packers - when i include the unused imports DataFrame, Panel in packers.py and run the test on test_packers.py it passes but when I exclude either of them or both from the packers module it fails. I am not too experienced with the import machinery but this seems a bit strange. There are a few more unused imports like Float64Index, Int64Index which do not seem to be causing the tests to fail when removing them. Is there a way to make the linter ignore unused imports (in which case I could just include them all) or is it critical that these are picked up ? TEST 1 (after including both DataFrame and Panel imports ) ==================================================================== pandas/tests/io/test_packers.py .........................................s.s.ss.s............s.s..s.s.sss.......... [100%] =========================================================== 71 passed, 12 skipped in 2.38 seconds =========================================================== TEST 2 (after excluding Panel import)
E KeyError: 'Panel' pandas/io/packers.py:667: KeyError |
msgpack needs the imports to correctly dessrialize things you cannot remove them |
Thanks, ok just pushed the imports but what do i do with the linter complaining about unused imports ? |
Can you merge master |
@jbrockmendel thanks |
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.
lgtm @jreback
pandas/io/json/json.py
Outdated
@@ -12,8 +12,9 @@ | |||
|
|||
from pandas.core.dtypes.common import is_period_dtype | |||
|
|||
from pandas import DataFrame, MultiIndex, Series, compat, isna, to_datetime | |||
from pandas.core.reshape.concat import concat | |||
from pandas import MultiIndex, compat, concat, isna, to_datetime |
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.
DataFrame/Series should be from pandas (same for all places)
@ryankarlos do we even need this PR any more? I think fixed by the implicit import PR you did earlier |
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.
@WillAyd I still think it would be useful to have this PR as Ive just noticed a few places which could still be importing from top level even after reverting all changes ... see comments below.
I fixed these now in latest commit
pandas/io/json/json.py
Outdated
from pandas import compat | ||
from pandas.core.frame import DataFrame | ||
from pandas.core.index import MultiIndex | ||
from pandas.core.missing import isna | ||
from pandas.core.reshape.concat import concat |
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.
like here
pandas/io/json/table_schema.py
Outdated
@@ -12,9 +12,9 @@ | |||
is_datetime64tz_dtype, is_integer_dtype, is_numeric_dtype, is_period_dtype, | |||
is_string_dtype, is_timedelta64_dtype) | |||
|
|||
from pandas import DataFrame | |||
from pandas.api.types import CategoricalDtype |
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.
@WillAyd and here
I would rather just boil this PR down to removing the blacklist items if no further changes to imports are required |
e0097f9
to
90b4383
Compare
So OK for me to completely reset changes in this PR as well and only keep mypy.ini changes ? |
Yes - the rest of the elements are arguably orthogonal at this point. Can be changed in separate more comprehensive PRs if we desire |
90b4383
to
e08ab93
Compare
Ok thanks - just pushed only mypy changes and reset everything else here and in #25936. Just waiting for tests to pass. |
Thanks @ryankarlos ! |
git diff upstream/master -u -- "*.py" | flake8 --diff