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

dataclass_transform: use regular dataclasses.field for field_specifiers #239

Merged

Conversation

noirbee
Copy link
Contributor

@noirbee noirbee commented Apr 19, 2023

cf https://docs.python.org/3/library/typing.html#typing.dataclass_transform: this is needed so that type checkers properly handle attributes using field(). Without this, any attribute using = field(...) is treated as if it were a default value by mypy, which makes it impossible to use its other arguments. This is even more problematic for marshmallow-dataclass since it makes it impossible to pass arguments to marshmallow using metadata=...

(note that this only affects users of mypy >= 1.1, since this is the release in which @dataclass_transform support was added)

IMHO this should probably be the default value in Python's typing module / typing_extensions but changing it in marshmallow-dataclass is probably easier/faster to get it to work ASAP :)

@noirbee
Copy link
Contributor Author

noirbee commented Apr 19, 2023

The tests for Python3.6 fail because it uses typing-extensions v4.1.1, where dataclass_transform has a field_descriptors argument. In v4.2.0 dataclass_transform was updated to rename it field_specifiers instead, while also dropping Python3.6 support ( https://github.com/python/typing_extensions/blob/main/CHANGELOG.md#release-420-april-17-2022 )

I'm trying to support it by inspecting the function's signature, but at this point I wonder if it's worth it vs bumping typing-extension to >=4.2 and dropping support for python3.6, which has now been EOLed for over a year.

@dairiki
Copy link
Collaborator

dairiki commented Apr 19, 2023

I'm trying to support it by inspecting the function's signature, but at this point I wonder if it's worth it vs bumping typing-extension to >=4.2 and dropping support for python3.6, which has now been EOLed for over a year.

I'd be all for dropping python3.6 support. However, the choice on whether to do that is up to the project owner (@lovasoa), who, the last time the issue was brought up, still thought it was worth keeping python 3.6 support. (It some months later now, though, so maybe his answer has changed?)

In any case, for now, how about just something like:

if sys.version_info >= (3, 11):
    from typing import dataclass_transform
elif sys.version_info >= (3,7):
    from typing_extensions import dataclass_transform
else:
    # @dataclass_transform() only helps us with mypy>=1.1 which is only runs under python>=3.7
    def dataclass_transform(**kwargs):
        return lambda cls: cls

@lovasoa
Copy link
Owner

lovasoa commented Apr 19, 2023

Last time we talked about it, we checked the stats and it seemed like there was still a non negligible amount of downloads from 3.6

Looks like it is almost zero now:
https://pypistats.org/packages/marshmallow-dataclass

So I'm okay with dropping support. We just need to make the next release a new major version.

@dairiki
Copy link
Collaborator

dairiki commented Apr 19, 2023

So I'm okay with dropping support. We just need to make the next release a new major version.

Since (I think) the workaround to support py36 in this PR is simple and clean enough, I say we go for that.


Also, I would think that when/if we do drop py36 support, bumping the minor version is sufficient to cover that.
(There is arguably no backward-incompatible API change associated with dropping support of old pythons.)
In any case, so long as we update Requires-Python appropriately, there is no breakage for users of python 3.6. For them, pip will continue, by default, to install the highest available version that claims to support python 3.6.

cf https://docs.python.org/3/library/typing.html#typing.dataclass_transform:
this is needed so that type checkers properly handle attributes using
field(). Without this, any attribute using `= field(...)` is treated as
if it were a default value by mypy, which makes it impossible to use
its other arguments. This is even more problematic for
marshmallow-dataclass since it makes it impossible to pass arguments
to marshmallow using `metadata=...`
@noirbee noirbee force-pushed the fix/dataclass-transform-field-specifiers branch from f98f4f2 to 519e695 Compare April 20, 2023 06:58
@noirbee
Copy link
Contributor Author

noirbee commented Apr 20, 2023

Updated the PR with @dairiki 's suggestions; using a dummy @dataclass_transform() works much better / is simpler than the approach I first tried (using inspect.signature() / and generating a kwargs dict on the fly), thanks !

Copy link
Owner

@lovasoa lovasoa left a comment

Choose a reason for hiding this comment

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

@dairiki , feel free to merge :)

@dairiki dairiki merged commit 5f7ffd0 into lovasoa:master Apr 20, 2023
7 checks passed
@dairiki
Copy link
Collaborator

dairiki commented Apr 20, 2023

Merged and released as 8.5.13.

Thank you, @noirbee !

@noirbee noirbee deleted the fix/dataclass-transform-field-specifiers branch April 21, 2023 09:31
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

3 participants