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.

Sign up for GitHub

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

Jump to bottom

CLN: tests/extension/json/test_json.py typefix #28994

Closed
wants to merge 19 commits into from
Closed

CLN: tests/extension/json/test_json.py typefix #28994

wants to merge 19 commits into from

Conversation

lukasbk
Copy link
Contributor

@lukasbk lukasbk commented Oct 15, 2019 •
edited
Loading

The error is due to the fact that the builtins.staticmethod type defined in extension.base.base.BaseExtensionTests does not match the function type in extension.json.test_json.BaseJSON. I don't see a possibility for a proper type hint fix without changing the structure of the code. One way would be to replace the call of staticmethod by function definitions that call the corresponding methods.
Here I propose an easier suppression of the error using type: Any.
What do you think?

@lukasbk
CLN: fix mypy errors in pandas/tests/extension/test_numpy.py pandas-d…
a19c844 
…ev#28926
@lukasbk
Applied suggestions, PR also fixes errors in test_sparse.py
28b7120
@lukasbk
Adjusted imports
008fe48
@lukasbk
CLN: added type hint for mypy to test_coercion.py
217adc8
@lukasbk
Merge remote-tracking branch 'upstream/master' into test_coercion_typ…
ea903c9 
…efix
@lukasbk
Fixed mypy error in test_json.py
8e8daed 
The error is due to the fact that the builtins.staticmethod type defined
 in extension.base.base.BaseExtensionTests does not match the function
type in extension.json.test_json.BaseJSON. I don't see a possibility for
a type hint fix without changing the structure of the code. One way
would be to replace the call of staticmethod by function definitions
that call the corresponding methods.
Here I propose an easier suppression of the error using type: Any.
What do you think?
@lukasbk
reformatting
524ce17
@simonjayhawkins simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Oct 15, 2019
simonjayhawkins
simonjayhawkins requested changes Oct 15, 2019
View reviewed changes
Copy link
Member

@simonjayhawkins simonjayhawkins left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@lukasbk Thanks for the PR.

This PR also has the changes from #28990, can you keep these separate.

also see #28988 re discussion on solving Definition of "assert_series_equal" in base class "BaseJSON" is incompatible with definition in base class "BaseExtensionTests" errors.

@simonjayhawkins simonjayhawkins mentioned this pull request Oct 15, 2019
Closed
38 tasks
@jbrockmendel
Copy link
Member

@lukasbk can you rebase

@pep8speaks
Copy link

pep8speaks commented Oct 20, 2019 •
edited
Loading

Hello @lukasbk! 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-12-15 18:57:28 UTC

simonjayhawkins
simonjayhawkins requested changes Oct 21, 2019
View reviewed changes
pandas/tests/extension/base/base.py Outdated
@@ -2,8 +2,18 @@


class BaseExtensionTests:
@staticmethod
def assert_equal(left, right, **kwargs):
return tm.assert_equal(left, right, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

don't need the return

pandas/tests/extension/base/base.py Outdated
assert_extension_array_equal = staticmethod(tm.assert_extension_array_equal)
@staticmethod
def assert_series_equal(left, right, **kwargs):
return tm.assert_series_equal(left, right, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

same

pandas/tests/extension/base/base.py Outdated

@staticmethod
def assert_frame_equal(left, right, **kwargs):
return tm.assert_frame_equal(left, right, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

same

pandas/tests/extension/base/base.py Outdated

@staticmethod
def assert_extension_array_equal(left, right, **kwargs):
return tm.assert_extension_array_equal(left, right, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

same

@simonjayhawkins simonjayhawkins added this to the 1.0 milestone Oct 21, 2019
WillAyd
WillAyd requested changes Oct 23, 2019
View reviewed changes
pandas/tests/extension/base/base.py
@@ -2,8 +2,18 @@


class BaseExtensionTests:
@staticmethod
Copy link
Member

Choose a reason for hiding this comment

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

Can you get rid of these methods altogether and just import / use as function within tests?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think so, since some tests need to override these

Copy link
Member

Choose a reason for hiding this comment

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

Hmm OK - so was this change required? Looks like using tm.assert_* in the tests now?

@WillAyd
Copy link
Member

WillAyd commented Nov 8, 2019

@lukasbk looks like a merge conflict; can you fix up and address comments?

@WillAyd WillAyd mentioned this pull request Nov 8, 2019
Closed
5 tasks
simonjayhawkins
simonjayhawkins reviewed Nov 13, 2019
View reviewed changes
pandas/tests/extension/base/base.py Outdated
@@ -2,8 +2,18 @@


class BaseExtensionTests:
@staticmethod
def assert_equal(left, right, **kwargs):
Copy link
Member

Choose a reason for hiding this comment

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

AFAICT it's only assert_series_equal and assert_frame_equal that currently need to be overridden in specific extension array tests, so assert_equal and assert_extension_array_equal don't need to be changed at this time.

@lukasbk
Resolve merge in setup.cfg and reduce base.py
a7fcf3f 
assert_equal and assert_extension_array_equal were not needed to be overwritten. Instead they can be directly accessed from testing.
@lukasbk
Fix stata files
d5856b8
@lukasbk
Change more references to tm
2da8bad
@jreback
Copy link
Contributor

jreback commented Nov 16, 2019

looks ok to me. I am bit concerned that we are now changing back to using tm as we do want the ability to override on these tests, but I guess that mypy will check this now.

@jreback
Copy link
Contributor

jreback commented Nov 20, 2019

can you rebase

@WillAyd
Copy link
Member

WillAyd commented Dec 13, 2019

@lukasbk can you fix up the merge conflict? I think we can get this in if so

@datapythonista
Copy link
Member

@lukasbk the CI didn't pass, and this is conflicting again.

Do you mind merging master again, making sure the CI passes, and pinging us to get this merged quickly after that please? Thanks!

@jreback jreback removed this from the 1.0 milestone Jan 1, 2020
@WillAyd
Copy link
Member

WillAyd commented Jan 16, 2020

@lukasbk can you fix up merge conflict? I think this was close

@simonjayhawkins
Copy link
Member

closing as stale. cc @SaturnFromTitan

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@WillAyd WillAyd WillAyd requested changes

@simonjayhawkins simonjayhawkins simonjayhawkins requested changes

Assignees
No one assigned
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@lukasbk @jbrockmendel @pep8speaks @WillAyd @jreback @datapythonista @simonjayhawkins