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

Full type annotations #199

Merged
merged 12 commits into from
Aug 22, 2020
Merged

Full type annotations #199

merged 12 commits into from
Aug 22, 2020

Conversation

staticdev
Copy link
Contributor

@staticdev staticdev commented Aug 9, 2020

  • Update mypy configs.
  • Add typing information.
  • Add an empty py.typed file along with the package, to let static checkers know that this package provides type annotations.

Closes #152

@staticdev staticdev marked this pull request as draft August 9, 2020 22:22
@staticdev staticdev marked this pull request as ready for review August 9, 2020 22:33
@staticdev
Copy link
Contributor Author

@nicoddemus I did a bunch of improvements over my initial PR #194

Copy link
Member

@nicoddemus nicoddemus left a comment

Choose a reason for hiding this comment

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

Thanks @svenstaro for pursuing this!

However in order to close #152 we need to:

  • Properly annotate all public methods that are part of the API.
  • Add an empty py.typed file along with the package, to let static checkers know that this package provides type annotations.
  • Mention this in the README.

So I'm OK with merging this as is because it annotates a few functions already, but in order to close #152 we have more work to do here. 👍

src/pytest_mock/plugin.py Outdated Show resolved Hide resolved
src/pytest_mock/plugin.py Outdated Show resolved Hide resolved
Co-authored-by: Bruno Oliveira <[email protected]>
@staticdev staticdev marked this pull request as draft August 12, 2020 01:45
@staticdev
Copy link
Contributor Author

@nicoddemus there is a problem with the changes you suggested. Since mock is imported via _get_mock_module and could be two different modules, it is not being resolved by type-check. Do you have any workaround for that? If not I suggest going back to Any or choose one of them to be imported in the beginning.

PS.: I have also added py.typed.

@nicoddemus
Copy link
Member

Hey @staticdev !

If not I suggest going back to Any or choose one of them to be imported in the beginning.

Returning Any there will prevent users from benefit from type checking themselves, which defeats the point a bit.

Do you have any workaround for that? If not I suggest going back to Any or choose one of them to be imported in the beginning.

We can declare those functions as returning unittest.mock.MagicMock and use typing.cast, for example:

    def spy(self, obj: object, name: str) -> unittest.mock.MagicMock:
        ...
        spy_obj = self.patch.object(obj, name, side_effect=wrapped, autospec=autospec)
        spy_obj.spy_return = None
        spy_obj.spy_exception = None
        return cast(unittest.mock.MagicMock, spy_obj)

(We also need to import unittest.mock at the top level)

This works because both the standalone mock and unittest.mock implement the same interface, which is enough for the type checker.

Also we should document the other public functions with full arguments, for example patch, multiple, etc, like it was done in the pytest_mock typeshed, probably a few overloads too.

PS.: I have also added py.typed.

Thanks! We however also need to include that file in the package to signal the type checker that this module provides type annotations: https://www.python.org/dev/peps/pep-0561/#id18.

I understand this might seem quite a bit more work than it was thought initially, but I suspect it is needed to make the type annotations as useful as possible for users.

I plan to tackle the issues above soon in case you don't have to work on them, probably in the next few days.

Thanks again all the work done so far.

@staticdev
Copy link
Contributor Author

Hi @nicoddemus,

About your last message:

  1. I added the new changes to spy() you suggested, but with we have the errors:
src/pytest_mock/plugin.py:98: error: Too many arguments for "__getattribute__"
of "object"  [call-arg]
                    value = obj.__getattribute__(obj, name)
                            ^
src/pytest_mock/plugin.py:98: error: Argument 1 to "__getattribute__" of
"object" has incompatible type "object"; expected "str"  [arg-type]
                    value = obj.__getattribute__(obj, name)
                                                 ^
  1. I tried documenting more methods using as a base https://github.com/python/typeshed/blob/master/third_party/3/pytest_mock/plugin.pyi. But, _Patcher.object() and _Patcher.object(multiple) have significantly different signatures and might need some refactors. Also, parse_ini_boolean() types are correct, but type-checks are still complaining about the code logic:
src/pytest_mock/plugin.py:411: error: Incompatible return value type (got
"Union[bool, str]", expected "bool")  [return-value]
            return value
                   ^
src/pytest_mock/plugin.py:413: error: Item "bool" of "Union[bool, str]" has no
attribute "lower"  [union-attr]
            return {"true": True, "false": False}[value.lower()]
                                                  ^
  1. About py.typed https://www.python.org/dev/peps/pep-0561/#id18. I hope this fixed that.

@nicoddemus
Copy link
Member

I see, thanks @staticdev!

I will take a look at the problems later then. 👍

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.

add type annotations
2 participants