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

pandas-dev#28926 Mypy fix/extension json array #28989

Closed
wants to merge 3 commits into from
Closed

pandas-dev#28926 Mypy fix/extension json array #28989

wants to merge 3 commits into from

Conversation

aaditya-panik
Copy link
Contributor

@aaditya-panik aaditya-panik commented Oct 15, 2019

ref: #28926

  • tests added / passed
  • passes black pandas
  • passes git diff upstream/master -u -- "*.py" | flake8 --diff

@simonjayhawkins simonjayhawkins added the Typing type annotations, mypy/pyright type checking label Oct 15, 2019
@@ -27,7 +27,7 @@
class JSONDtype(ExtensionDtype):
type = abc.Mapping
name = "json"
na_value = UserDict()
Copy link
Member

Choose a reason for hiding this comment

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

@simonjayhawkins i know ive asked you this before, but why cant mypy figure this out on its own?

Copy link
Member

Choose a reason for hiding this comment

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

the revealed type for na_value is Revealed type is 'collections.UserDict[Any, Any]'.

mypy asks for more info to narrow the Any

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.

@aaditya-panik Thanks for the PR.

@@ -27,7 +27,7 @@
class JSONDtype(ExtensionDtype):
type = abc.Mapping
name = "json"
na_value = UserDict()
na_value = UserDict() # type: UserDict
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 type this as Mapping[<type>, <type>]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So since this test is for JSONDtype we are looking at a Mapping[str, Any] correct? A custom JSON Type was/is discussed in python/typing#182

Copy link
Member

Choose a reason for hiding this comment

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

I think UserDict[str, Any] works

Copy link
Member

Choose a reason for hiding this comment

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

this will produce TypeError: 'ABCMeta' object is not subscriptable using python 3.6 syntax and will require a workaround, see https://mypy.readthedocs.io/en/latest/common_issues.html#using-classes-that-are-generic-in-stubs-but-not-at-runtime so probably best to use Mapping from typing.

Copy link
Member

Choose a reason for hiding this comment

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

I think UserDict[str, Any] works

This is OK. probably better than Mapping. just needs to be quoted for py3.6

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

Can you merge master, address comment and see if we can get CI green?

@@ -27,7 +28,7 @@
class JSONDtype(ExtensionDtype):
type = abc.Mapping
name = "json"
na_value = UserDict()
na_value = UserDict() # type: Mapping[type, type]
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 type should be used for subscripts; I think just Mapping[str, Any] is fine

Copy link
Member

Choose a reason for hiding this comment

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

will need to change to py3.6 syntax.

@alimcmaster1
Copy link
Member

@aaditya-panik are you still working on this? Looks like there are a final few comments to address

@WillAyd
Copy link
Member

WillAyd commented Dec 17, 2019

Looks like this has gone stale so closing for now but ping if you'd like to pick back up and address comments

@WillAyd WillAyd closed this Dec 17, 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.

5 participants