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 broken user types after typing-inspect 0.8.0 #207

Closed
wants to merge 2 commits into from
Closed

Fix broken user types after typing-inspect 0.8.0 #207

wants to merge 2 commits into from

Conversation

vit-zikmund
Copy link

Since the recent release of typing-inspect 0.8.0, it's is_new_type() check expects the new type to come from the typing or typing_extensions module. When using local NewType, this condition isn't met.

The fix makes use of the typing.NewType and only adds the marshmallow_dataclass specific fields.

Since the recent release of `typing-inspect` 0.8.0, it's `is_new_type()` check expects the new type to come from the `typing` or `typing_extensions` module. When using local `NewType`, this condition isn't met.

The fix makes use of the `typing.NewType` and only adds the `marshmallow_dataclass` specific fields.
@vit-zikmund
Copy link
Author

vit-zikmund commented Aug 20, 2022

I made the change from github as it seemed pretty straightforward, but oh my, was I wrong. When running the local commit checks i get:

mypy.....................................................................Failed
- hook id: mypy
- exit code: 1

marshmallow_dataclass/__init__.py:809: error: Argument 1 to NewType(...) must be a string literal  [misc]
marshmallow_dataclass/__init__.py:809: error: Variable "typ" is not valid as a type  [valid-type]
marshmallow_dataclass/__init__.py:809: note: See https://mypy.readthedocs.io/en/stable/common_issues.html#variables-vs-type-aliases
Found 2 errors in 1 file (checked 1 source file)

Adding some ignore comments fixed the warning, but I don't have a clue if that's feasible.

@vit-zikmund
Copy link
Author

FYI, I have no clue why the 3.8 test failed here. Can't reproduce at my place (Fedora 36) with:

python3.8 -m venv venv38
. venv38/bin/activate
pip install --upgrade pip setuptools
pip install '.[dev]'
pytest
=================================================================== test session starts ====================================================================
platform linux -- Python 3.8.13, pytest-7.1.2, pluggy-1.0.0
rootdir: /home/tlwhitec/git/github.com/lovasoa/marshmallow_dataclass, configfile: pyproject.toml
plugins: typeguard-2.13.3, mypy-plugins-1.10.0
collected 94 items                                                                                                                                         

tests/test_attribute_copy.py ....                                                                                                                    [  4%]
tests/test_city_building_examples.py ..                                                                                                              [  6%]
tests/test_class_schema.py ..............                                                                                                            [ 21%]
tests/test_collection.py ...................s                                                                                                        [ 42%]
tests/test_field_for_schema.py .......................                                                                                               [ 67%]
tests/test_forward_references.py ..........                                                                                                          [ 77%]
tests/test_mypy.yml ...                                                                                                                              [ 80%]
tests/test_optional.py ...                                                                                                                           [ 84%]
tests/test_post_load.py ..                                                                                                                           [ 86%]
tests/test_postdump.py ..                                                                                                                            [ 88%]
tests/test_union.py ...........                                                                                                                      [100%]

============================================================== 93 passed, 1 skipped in 3.29s ===============================================================

@vit-zikmund
Copy link
Author

Investigating the error a bit I found it could be related to the way how the package is installed for the test:

pip install --pre -e '.[dev]'

This differs from the contributing guide which has only:

pip install '.[dev]'

Playing with the args, found the -e (editable) installation is a problem for mypy. Without -e, the tests end well. So maybe giving up on it for the github workflow might be a possibility? FYI @lovasoa

@lovasoa
Copy link
Owner

lovasoa commented Aug 22, 2022

Why is -e a problem for mypy ? Isn't it the recommended way to install things in development mode ?

@vit-zikmund
Copy link
Author

@lovasoa It may as well be, when you're running local tests on your workstation (so you don't have to pip install each time you change something in the code, but in case of the ephemeral nature of the github workflows, this seems to make little sense (maybe except for saving a couple ms for copying the package over to site-packages).

But the problem here is more mundane, because when there's -e, all local PR checks die on the same error (it's not just this PR). I can reproduce that locally. If I omit that -e, the tests pass. Sadly I'm not even an apprentice regarding mypy, so I don't have a clue why this breaks with -e. I came here for another reason 😏

@vit-zikmund
Copy link
Author

vit-zikmund commented Aug 22, 2022

Sorry, I just couldn't let it go :) It seems mypy just has some trouble figuring out the path to the package when it's installed with -e. Seems pytest doesn't use the current directory for module search by default. When I install with -e, I can still get a successful test with:

PYTHONPATH=$(pwd) pytest

or maybe even better with:

python -m pytest

which also works well. Maybe the last one is the most foolproof (it's also one of the documented ways).

@mivade
Copy link

mivade commented Aug 23, 2022

Personally I don't think editable installs should be used on CI since that can miss things like data files not getting installed in the package (I don't believe that's an issue here though).

@vit-zikmund
Copy link
Author

PR for fixing the mypy checks: #209 FYI @lovasoa @mivade

dairiki added a commit to dairiki/marshmallow_dataclass that referenced this pull request Sep 23, 2022
This prevents issues when marshmallow_dataclass is installed
in editable mode. (See lovasoa#209, lovasoa#207.)
dairiki added a commit to dairiki/marshmallow_dataclass that referenced this pull request Sep 23, 2022
This prevents issues when marshmallow_dataclass is installed
in editable mode. (See lovasoa#209, lovasoa#207.)
dairiki added a commit to dairiki/marshmallow_dataclass that referenced this pull request Sep 23, 2022
lovasoa pushed a commit that referenced this pull request Sep 23, 2022
* Explicitly set PYTHONPATH=. in mypy tests

This prevents issues when marshmallow_dataclass is installed
in editable mode. (See #209, #207.)

* Don't fail tests upon warnings from external packages.

E.g. marshmallow<3.15 generates a deprecation warning from distutils.
There's no reason that should cause a test failure for us.

* Update black target-version config.

I have no idea whether this actually affects anything, but
might as well keep it up-to-date.

* Fix typing of Union._serialize parameter

This tracks a change in type of the `attr` parameter to
`Field._serialize` made in marshmallow 3.18.0.

Here we also switch to using keyword args to typeguard.check_type
in an attempt to protect against upcoming
[changes in its signature](https://typeguard.readthedocs.io/en/latest/api.html#typeguard.check_type).

* Cherry-pick PR #207: Fix broken user types after typing-inspect 0.8.0

This is cherry-picks PR#207 by @vit-zikmund
@dairiki
Copy link
Collaborator

dairiki commented Sep 23, 2022

I have cherry-picked this into #211 which has been merged to master... closing.

And thank you!

@dairiki dairiki closed this Sep 23, 2022
@vit-zikmund vit-zikmund deleted the patch-1 branch September 23, 2022 21:00
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.

None yet

4 participants