Skip to content
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

Fix mypy error in pandas/tests.indexes.test_base.py #29188

Merged
merged 14 commits into from
Nov 27, 2019
Merged

Fix mypy error in pandas/tests.indexes.test_base.py #29188

merged 14 commits into from
Nov 27, 2019

Conversation

rushabh-v
Copy link
Contributor

@rushabh-v rushabh-v commented Oct 23, 2019

@WillAyd
Copy link
Member

WillAyd commented Oct 23, 2019

What error does this give?

@WillAyd WillAyd added the Typing type annotations, mypy/pyright type checking label Oct 23, 2019
@rushabh-v
Copy link
Contributor Author

Hello @WillAyd
Thanks for responding.
It was giving error: Need type annotation for 'x'.

@WillAyd WillAyd added this to the 1.0 milestone Oct 24, 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.

OK lgtm. Can you merge master one more time and push? Fixed some things with CI recently

@simonjayhawkins

@rushabh-v
Copy link
Contributor Author

I have merged it again. Please review the changes.
@WillAyd @simonjayhawkins

@simonjayhawkins
Copy link
Member

simonjayhawkins commented Oct 25, 2019

@rushabh-v
Copy link
Contributor Author

I have started working on them.

@rushabh-v
Copy link
Contributor Author

Hello @simonjayhawkins @WillAyd
It is giving the error: Slice index must be an integer or None.
This error is coming from mypy packages. And I think there is a mypy version conflict. Because this bug is solved with (python/mypy#2410)
The lines giving this error are (https://github.com/RushabhVasani/pandas/blob/fcddcfcda2de3306191067afe19bc86859665091/pandas/tests/indexes/test_base.py#L1741-L1756)
So I tried to run the same code on my local machine. And it all works fine. The code I run was,

@pytest.mark.parametrize( 
       "in_slice,expected",
        [
            (pd.IndexSlice[::-1], "yxdcb"),
            (pd.IndexSlice["b":"y":-1], ""),
            (pd.IndexSlice["b"::-1], "b"),
            (pd.IndexSlice[:"b":-1], "yxdcb"),
            (pd.IndexSlice[:"y":-1], "y"),
            (pd.IndexSlice["y"::-1], "yxdcb"),
            (pd.IndexSlice["y"::-4], "yb"),
            # absent labels
            (pd.IndexSlice[:"a":-1], "yxdcb"),
            (pd.IndexSlice[:"a":-2], "ydb"),
            (pd.IndexSlice["z"::-1], "yxdcb"),
            (pd.IndexSlice["z"::-3], "yc"),
            (pd.IndexSlice["m"::-1], "dcb"),
            (pd.IndexSlice[:"m":-1], "yx"),
            (pd.IndexSlice["a":"a":-1], ""),
            (pd.IndexSlice["z":"z":-1], ""),
            (pd.IndexSlice["m":"m":-1], ""),
        ],
    )

And the output it gave was,

MarkDecorator(mark=Mark(name='parametrize', args=('in_slice,expected', [(slice(None, None, -1), 'yxdcb'), (slice('b', 'y', -1), ''), (slice('b', None, -1), 'b'), (slice(None, 'b', -1), 'yxdcb'), (slice(None, 'y', -1), 'y'), (slice('y', None, -1), 'yxdcb'), (slice('y', None, -4), 'yb'), (slice(None, 'a', -1), 'yxdcb'), (slice(None, 'a', -2), 'ydb'), (slice('z', None, -1), 'yxdcb'), (slice('z', None, -3), 'yc'), (slice('m', None, -1), 'dcb'), (slice(None, 'm', -1), 'yx'), (slice('a', 'a', -1), ''), (slice('z', 'z', -1), ''), (slice('m', 'm', -1), '')]), kwargs={}))

So I think there is a mypy version conflict.

@simonjayhawkins
Copy link
Member

So I think there is a mypy version conflict.

yes these errors are not reported in 0.730. It was a typeshed fix though, see python/typeshed#3024 which was pulled into mypy for version 0.730

we are currently pinned to 0.720 on ci.

@rushabh-v
Copy link
Contributor Author

we are currently pinned to 0.720 on ci.

Okay, then what should be done now.

@simonjayhawkins
Copy link
Member

we are currently pinned to 0.720 on ci.

Okay, then what should be done now.

I guess two options.

  1. seperate PR to bump mypy to 0.730 as a precursor to this
  2. add # type: ignore with comments for error message and link to typeshed issue.

eg.

# https://github.com/python/mypy/issues/4975
# error: Incompatible return value type (got "Tuple[List[Sequence[str]],
# List[Sequence[str]]]", expected "Tuple[List[Tuple[str, ...]],
# List[Tuple[str, ...]]]")
return head, tail # type: ignore

@WillAyd any preference?

@simonjayhawkins
Copy link
Member

@rushabh-v can you merge master. mypy has been bumped to 0.730 on ci.

@rushabh-v
Copy link
Contributor Author

It is giving the same error(Slice index must be an integer or None) again.
@simonjayhawkins

@WillAyd
Copy link
Member

WillAyd commented Nov 19, 2019

Yea I see the same locally - can you investigate further @rushabh-v ?

@rushabh-v
Copy link
Contributor Author

rushabh-v commented Nov 19, 2019

Yes, I am looking into it.

@rushabh-v
Copy link
Contributor Author

I am sorry, I mistakenly committed directly to master.

@rushabh-v
Copy link
Contributor Author

rushabh-v commented Nov 27, 2019

It is giving this error for every version of mypy on my local machine. Even in 0.740.

I guess two options.

  1. seperate PR to bump mypy to 0.730 as a precursor to this
  1. add # type: ignore with comments for error message and link to typeshed issue.

Should we go with the 2nd option? @simonjayhawkins @WillAyd
I have checked locally adding, #type: ignore works fine on mypy 0.730.

@simonjayhawkins
Copy link
Member

replacing (pd.IndexSlice["b":"y":-1], ""), with (pd.IndexSlice[slice("b", "y", -1)], ""), silences mypy. so it appears that the current mypy errors are not directly related to/ been solved by python/typeshed#3024

the mypy issue, python/mypy#2410 is still open, so a # type: ignore with a comment referencing the mypy issue is probably the most practical solution here.

@pep8speaks
Copy link

pep8speaks commented Nov 27, 2019

Hello @rushabh-v! 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-11-27 12:05:49 UTC

Copy link
Member

@simonjayhawkins simonjayhawkins left a comment

Choose a reason for hiding this comment

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

@rushabh-v Thanks for sticking with this. @WillAyd

@WillAyd WillAyd merged commit 6d3a08a into pandas-dev:master Nov 27, 2019
@WillAyd
Copy link
Member

WillAyd commented Nov 27, 2019

Thanks @rushabh-v

keechongtan added a commit to keechongtan/pandas that referenced this pull request Nov 29, 2019
…ndexing-1row-df

* upstream/master: (32 commits)
  DEPR: Series.cat.categorical (pandas-dev#29914)
  DEPR: infer_dtype default for skipna is now True (pandas-dev#29876)
  Fix broken asv (pandas-dev#29906)
  DEPR: Remove weekday_name (pandas-dev#29831)
  Fix mypy errors for pandas\tests\series\test_operators.py (pandas-dev#29826)
  CI: Setting path only once in GitHub Actions (pandas-dev#29867)
  DEPR: passing td64 data to DTA or dt64 data to TDA (pandas-dev#29794)
  CLN: remove unsupported sparse code from io.pytables (pandas-dev#29863)
  x.__class__ TO type(x) (pandas-dev#29889)
  DEPR: ftype, ftypes (pandas-dev#29895)
  REF: use named funcs instead of lambdas (pandas-dev#29841)
  Correct type inference for UInt64Index during access (pandas-dev#29420)
  CLN: follow-up to 29725 (pandas-dev#29890)
  CLN: trim unnecessary code in indexing tests (pandas-dev#29845)
  TST added test for groupby agg on mulitlevel column (pandas-dev#29772) (pandas-dev#29866)
  mypy fix (pandas-dev#29891)
  Typing annotations (pandas-dev#29850)
  Fix mypy error in pandas/tests.indexes.test_base.py (pandas-dev#29188)
  CLN: remove never-used kwargs, make kwargs explicit (pandas-dev#29873)
  TYP: Added typing to __eq__ functions (pandas-dev#29818)
  ...
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
proost pushed a commit to proost/pandas that referenced this pull request Dec 19, 2019
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants