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 errors #194

Merged
merged 7 commits into from
Aug 6, 2020
Merged

Fix mypy errors #194

merged 7 commits into from
Aug 6, 2020

Conversation

staticdev
Copy link
Contributor

@staticdev staticdev commented Jun 25, 2020

Closes #193

@staticdev
Copy link
Contributor Author

@nicoddemus if you agree with this drop in 3.5, I can proceed with more strict typing support PRs.

@nicoddemus
Copy link
Member

Hi @staticdev,

First of all, thanks for tackling this!

Notice this PR drops support for Python 3.5.

Can you explain why is that? I noticed you added an annotation to _mock_module_originals, is because of that? Can't we use a #type: comment instead?

@staticdev
Copy link
Contributor Author

staticdev commented Jun 25, 2020

@nicoddemus you are right. It is an alternative solution. This comment makes mypy just ignore the type. By doing the way I did: _mock_module_originals: Dict[str, Any] mypy is actively resolving the types inside the script and does other chain checks that are consequences of this type. If you prefer I could change that to ignore, but there is less benefits of type-hinting checks with that.

@nicoddemus
Copy link
Member

If you prefer I could change that to ignore, but there is less benefits of type-hinting checks with that.

  1. Drop Python 3.5 now and get full type-checking (merge as is and release now).
  2. Use a type-hinting comment and get less benefits from type-checking (update the PR and release now).
  3. Hold on to the PR until Python 3.5 is EOL to make the release.

The problem is that I wouldn't like to drop support for 3.5 just yet without a very strong reason, so I'm 👎 on 1).

What do you think?

@staticdev
Copy link
Contributor Author

staticdev commented Jun 26, 2020

If you prefer I could change that to ignore, but there is less benefits of type-hinting checks with that.

1. Drop Python 3.5 now and get full type-checking (merge as is and release now).

2. Use a type-hinting comment and get less benefits from type-checking (update the PR and release now).

3. Hold on to the PR until Python 3.5 is EOL to make the release.

The problem is that I wouldn't like to drop support for 3.5 just yet without a very strong reason, so I'm -1 on 1).

What do you think?

I think 3.5 EOL is just around the corner. You can see that here.

There is no problem on holding a release until then, but we could maybe create pre-releases and getting ready for that?

Another idea is to create a new major version and put a version compatibility table on the readme, like in: https://pypi.org/project/django-bootstrap-pagination/

@staticdev staticdev marked this pull request as draft August 6, 2020 17:12
@staticdev
Copy link
Contributor Author

staticdev commented Aug 6, 2020

@nicoddemus @jtpavlock I managed to get this PR working WITH 3.5, based on pytest 6.0.

This PR gives initial support to pytest-mock type-hinting.

There are two catches:

  • Pytest requires minimum of python 3.5.3 (you can see it here on Line 3).
  • Variable annotations are not supported by 3.5 (as you can see here). So some refactoring may be needed to solve _mock_module_originals reverted here.

@staticdev staticdev marked this pull request as ready for review August 6, 2020 17:56
@nicoddemus
Copy link
Member

Pytest requires minimum of python 3.5.3 (you can see it here on Line 3).

That should be fine, as it should only affect users that want to use mypy. I think reasonable to have that requirement in that case.

Variable annotations are not supported by 3.5 (as you can see here). So some refactoring may be needed to solve _mock_module_originals reverted here.

You can use type-hint comments for that:

_mock_module_originals = {} # type: Dict[str, Any]

pytest uses it a lot because of lack of variable type annotations due to Python 3.5. I will re-add that to your PR. 👍

@nicoddemus nicoddemus merged commit 747a1e6 into pytest-dev:master Aug 6, 2020
@nicoddemus
Copy link
Member

Thanks @staticdev!

@nicoddemus nicoddemus mentioned this pull request Aug 6, 2020
@staticdev staticdev mentioned this pull request Aug 9, 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.

Fix mypy errors
2 participants