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

Add misc types for mock and unittest.mock #3923

Merged
merged 1 commit into from
Apr 24, 2020
Merged

Conversation

mxr
Copy link
Contributor

@mxr mxr commented Apr 12, 2020

No description provided.

@hauntsaninja
Copy link
Collaborator

hauntsaninja commented Apr 12, 2020

Looks uncontroversial, CI failures apart.

  • check_consistent will want you to copy your changes to some other files.
  • the stubtest failure actually appears to be an issue with how mypy detects what is public in a stub; it seems that just the presence of a type annotation for __all__ is enough to throw it off. I'll attempt to fix this in mypy, but for now you can follow its suggestion of just removing the unittest.mock things it mentions from the stubtest whitelists.

@srittau
Copy link
Collaborator

srittau commented Apr 12, 2020

We don't type __all__ in stubs. __all__ can be used the same way it's used in implementation files, though, i.e. to list exported members.

@mxr
Copy link
Contributor Author

mxr commented Apr 13, 2020

@hauntsaninja thanks for the advice! I added those changes and pushed up

@srittau for some more context, I'm trying to work around this error:

$ mypy example.py
example.py:2: error: Module has no attribute "__all__"
example.py:3: error: Module has no attribute "__all__"

Where example.py is:

import unittest.mock, mock
print(unittest.mock.__all__)
print(mock.__all__)

And mypy is configured as follows:

$ mypy --version
mypy 0.770

$ cat setup.cfg
[mypy]
check_untyped_defs = true
disallow_any_generics = true
disallow_incomplete_defs = true
disallow_untyped_defs = true
no_implicit_optional = true

Some info about my system (code here):

  • sys.version: '3.6.5 (default, Apr 26 2019, 13:23:36) \n[GCC 4.2.1 Compatible Apple LLVM 10.0.1 (clang-1001.0.46.4)]'
  • os.name: 'posix'
  • sys.platform: 'darwin'
  • platform.system(): 'Darwin'
  • platform.python_implementation(): 'CPython'
  • sys.implementation.name: 'cpython'
  • sysconfig.get_platform(): 'macosx-10.14-x86_64'

I've seen __all__ typed this way in a few places so I thought this change was the right way to resolve the error.

@srittau
Copy link
Collaborator

srittau commented Apr 23, 2020

@mxr Sorry for the delay. I was wrong with my answer. We discussed __all__ behavior in srittau/peps#67 (which is not strictly about typeshed). The conclusion was that __all__ should be included if and only if the implementation has it, which is the case with unittest.mock. But it should actually list the exported items, like the implementation, not just type it.

@srittau
Copy link
Collaborator

srittau commented Apr 23, 2020

Thank you for the changes. Regarding the build failures:

@mxr
Copy link
Contributor Author

mxr commented Apr 23, 2020

Thanks for the review! Since enumerating the contents of __all__ seems unsupported would it be acceptable to type __all__ using the same technique as other files and then do a batch migration of all __all__'s in another PR?

@srittau
Copy link
Collaborator

srittau commented Apr 23, 2020

If this is a quick fix in pytype I'd prefer the "correct" solution, otherwise we can fall back to using the annotation only.

@rchen152
Copy link
Collaborator

I'll look into fixing our parser. In the meantime, would it work to write

__all__ = [
 ...
]

? (Remove the annotation and use a list instead of a tuple?) pytype should already support that because it's the same syntax we use for slots.

@mxr
Copy link
Contributor Author

mxr commented Apr 23, 2020

@rchen152 sure I'll do that. Thanks!

@JelleZijlstra
Copy link
Member

CI is still unhappy. The stubtest failures are unrelated and should go away if you rebase, but the flake8 errors look real.

@JelleZijlstra
Copy link
Member

Actually they're not; the names are all defined. #3940 should fix this, so PR should now pass completely.

@mxr
Copy link
Contributor Author

mxr commented Apr 24, 2020

Will rebase. Thanks again, everyone, for all the guidance and white glove service on this PR!

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Thanks for sticking it out despite the various errors unrelated to this PR!

@srittau srittau merged commit 31705fc into python:master Apr 24, 2020
grievejia pushed a commit to grievejia/typeshed that referenced this pull request Apr 24, 2020
CraftSpider added a commit to CraftSpider/typeshed that referenced this pull request May 27, 2020
vishalkuo pushed a commit to vishalkuo/typeshed that referenced this pull request Jun 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants