lovasoa / marshmallow_dataclass

Automatic generation of marshmallow schemas from dataclasses.
https://lovasoa.github.io/marshmallow_dataclass/html/marshmallow_dataclass.html
MIT License
456 stars 78 forks source link

dataclass_transform: use regular dataclasses.field for field_specifiers #239

Closed noirbee closed 1 year ago

noirbee commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.

noirbee commented 1 year ago

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 !

dairiki commented 1 year ago

Merged and released as 8.5.13.

Thank you, @noirbee !