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 support for lazy type annotations PEP563 (#112) #126

Merged
merged 2 commits into from May 13, 2021
Merged

Add support for lazy type annotations PEP563 (#112) #126

merged 2 commits into from May 13, 2021

Conversation

ydylla
Copy link
Contributor

@ydylla ydylla commented May 8, 2021

Hi @yukinarit,
I added basic support for from __future__ import annotations.

Do you have any ideas how we could tell pytest to skip test_lazy_type_evaluation.py when using python 3.6 since the annotations future does not exist there?

Closes #112

@ydylla ydylla requested a review from yukinarit May 8, 2021 21:29
@yukinarit
Copy link
Owner

Hi @ydylla
Thank you for the contribution!

Do you have any ideas how we could tell pytest to skip test_lazy_type_evaluation.py when using python 3.6 since the annotations future does not exist there?

Did you try pytest.mark.skipif? Sounds like this is what you are looking for.

import sys
@pytest.mark.skipif(sys.version_info < (3, 0),
                    reason="requires Python3")
def test_function():
    ...

https://docs.pytest.org/en/reorganize-docs/new-docs/user/skipping.html

@ydylla
Copy link
Contributor Author

ydylla commented May 10, 2021

Yes I am aware of pytest.mark.skipif, sorry I should have mention that.
The Problem is that the python 3.6 tests fail at from __future__ import annotations which has to be the first line in a file.
I think we need a way to ignore the complete test_lazy_type_evaluation.py file in the pipeline. But I have not much experience with github wokflows. An ugly hack could be to just delete the file before starting pytest 😄

I also forgot to mention that I added tests for forward references with this.
We can not really support forward references, but maybe we should mention the workaround in the docs/readme. Which basically is call deserialize & serialize only after all classes that are used in the forward reference are visible globally.

@dataclass
class A:
  v: 'B'

@serialize
@deserialize
@dataclass
class B:
  v: int

# B is now visible so we can setup A
serialize(A)
deserialize(A)

@yukinarit
Copy link
Owner

@ydylla
Oh! Now I understand the problem!

I found a workaround in pydantic project. Could you check it is applicable to our case?

kip_pre_37 = pytest.mark.skipif(sys.version_info < (3, 7), reason='testing >= 3.7 behaviour only')


@skip_pre_37
def test_postponed_annotations(create_module):
    module = create_module(
        # language=Python
        """
from __future__ import annotations
from pydantic import BaseModel
class Model(BaseModel):
    a: int
"""
    )
    m = module.Model(a='123')
    assert m.dict() == {'a': 123}

https://github.com/samuelcolvin/pydantic/blob/master/tests/test_forward_ref.py#L8-L24

Copy link
Owner

@yukinarit yukinarit left a comment

Choose a reason for hiding this comment

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

LGTM!

* also move SerdeError to compat.py
@codecov
Copy link

codecov bot commented May 11, 2021

Codecov Report

Merging #126 (4f0abac) into master (f6a36ba) will decrease coverage by 0.14%.
The diff coverage is 94.73%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #126      +/-   ##
==========================================
- Coverage   89.28%   89.13%   -0.15%     
==========================================
  Files          11       11              
  Lines        1017     1031      +14     
  Branches      224      227       +3     
==========================================
+ Hits          908      919      +11     
- Misses         70       71       +1     
- Partials       39       41       +2     
Impacted Files Coverage Δ
serde/compat.py 93.26% <94.44%> (-1.12%) ⬇️
serde/core.py 90.81% <100.00%> (-0.05%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f6a36ba...4f0abac. Read the comment docs.

@ydylla
Copy link
Contributor Author

ydylla commented May 11, 2021

Hi @yukinarit,
I tried to dynamically create modules like pydantic, but I could not get it to work properly.
Also I did not like that a lot of code only existed as string, which makes debugging very annoying.
Instead, I pushed a fix that just deletes the whole file for python 3.6.
Which was way easier and should probably be fine until 2021-12-23 when python 3.6 goes out of support anyways.

If you have no objections you can merge this.

@yukinarit
Copy link
Owner

yukinarit commented May 11, 2021

@ydylla
I found another workaround which sounds like a good fit. What do you think?

Create conftest.py in the project root.

import sys

if sys.version_info[:2] == (3, 6):
    collect_ignore = ["tests/test_lazy_type_evaluation.py"]

For more information, please check: https://docs.pytest.org/en/6.2.x/example/pythoncollection.html#customizing-test-collection

@ydylla
Copy link
Contributor Author

ydylla commented May 12, 2021

Thanks @yukinarit,
this is definitely better (the intended way to do this).

Copy link
Owner

@yukinarit yukinarit left a comment

Choose a reason for hiding this comment

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

LGTM!

@yukinarit yukinarit merged commit fe037a3 into yukinarit:master May 13, 2021
@yukinarit
Copy link
Owner

@ydylla
Thanks! Let me know if you need the latest pyserde on PyPI now.

@ydylla
Copy link
Contributor Author

ydylla commented May 13, 2021

Currently, not necessarily. I already use it via git installation.

@yukinarit yukinarit mentioned this pull request May 17, 2021
13 tasks
@ydylla ydylla deleted the lazy_types branch May 31, 2021 16:56
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.

Support PEP563 lazy type evaluation
2 participants