Add Annotated support #257

mvanderlee · 2024-03-09T21:56:17Z

Fixes #256

Moving away from NewType and instead add Annotated support.

mivade · 2024-03-09T23:29:27Z

I would recommend importing Annotated from typing_extensions for Python 3.8 support (at least for as long as that continues receiving security updates).

mvanderlee · 2024-03-20T16:45:00Z

Rebased, and retested.

mvanderlee · 2024-04-05T15:00:05Z

@lovasoa @dairiki Any idea when you'll have time to review this? I'd like to see this merged so I can rebase and finalize the generic dataclass PR #259

dairiki

I apologize for the delay. (Life has been crazy.)

The feature addition is a good one, I think. (Thank you!) And other than some style nits, I think it looks fine.

I think the addition of tox test configuration is long overdue. Ideally, it would be a separate PR, but since it's in here, already, I'm fine leaving it here. I do have a few comments about that.

dairiki · 2024-04-29T01:26:47Z

marshmallow_dataclass/__init__.py

+                or isinstance(arg, marshmallow.fields.Field)
+            ]
+            if marshmallow_annotations:
+                field = marshmallow_annotations[-1]


In the (admittedly esoteric case that) the annotations contain more than one marshmallow Field, here we use the last one and silently ignore the rest.
Is that the best (e.g. safest or most expected) behavior? Would it be better to raise an exception (e.g. a custom subclass of ValueError) in this case?

dairiki · 2024-04-29T03:52:55Z

tox.ini

+commands = pytest
+extras = dev
+set_env =
+  VIRTUALENV_DISCOVERY = pyenv


I was unfamilar with virtualenv-pyenv before this.

My initial reaction (I could maybe be convinced otherwise) is that I don't like it as a requirement for the tox test flow. A major advantage of using tox, in my view, is that one can use a globally-installed tox to run the tests, and let tox deal with all the details of setting up venvs. Adding virtualenv-pyenv as a requirement to run tox means that either virtualenv-pyenv must be globally installed, or we need to set up a venv just to bootstrap the tox process.

My normal workflow here is to use a globally installed tox and manually adjust the set of available python interpreters — either globally or locally — by running pyenv global [...] or pyenv local [...].

Does the use of virtualenv-pyenv buy one much compared to the manual use of pyenv global|local?

Does use of virtualenv-pyenv force the user to use pyenv? (I don't know.) If so, that's bad. (E.g. It would be nice to move to using tox to run the tests, e.g., in the CI testing.)

If we do want to keep using it, it should be added to [tox] requires (probably instead of including it in the tests extra dependencies in setup.py). (This causes tox to provision a bootstrap venv if necessary.)

dairiki · 2024-04-29T03:54:29Z

setup.py

    "docs": ["sphinx"],
    "tests": [
        "pytest>=5.4",
        # re: pypy: typed-ast (a dependency of mypy) fails to install on pypy
        # https://github.com/python/typed_ast/issues/111
        "pytest-mypy-plugins>=1.2.0; implementation_name != 'pypy'",
+        "tox>=4",
+        "virtualenv-pyenv",


I don't think either of these requirements belong here.

(I view tox as being in the same class as pip or virtualenv — it is a prerequisite of the package installation process. So — in the general case — it is installed before one can even start to install the package.)

(See note below on tox.ini.)

dairiki · 2024-04-29T04:56:34Z

marshmallow_dataclass/__init__.py

    if origin:
        # Override base_schema.TYPE_MAPPING to change the class used for generic types below
        type_mapping = base_schema.TYPE_MAPPING if base_schema else {}

+        if origin is Annotated:


Annotated is not a generic type (it is a special form). Handling it in a function named _field_for_generic_type could be confusing.

Admittedly, unions, which are also special forms, are already being handled in this function.

I'm not sure ... perhaps it would be better to handle Annotated (and union types) alongside where (the now-deprecated) NewType is handled in _field_for_schema. (New private functions, e.g., _field_for_annotated_type, _field_for_union_type could be introduced, if needed, to limit the complexity of _field_for_schema.)

dairiki · 2024-04-29T05:04:47Z

tox.ini

@@ -0,0 +1,11 @@
+[tox]
+requires =
+    tox>=4


Either add virtualenv-pyenv to requires, or remove usage of it. (See below.)

dairiki · 2024-04-29T16:08:45Z

README.md

@@ -247,28 +247,25 @@ class Sample:

 See [marshmallow's documentation about extending `Schema`](https://marshmallow.readthedocs.io/en/stable/extending.html).

-### Custom NewType declarations


We should probably leave at least some documentation for NewType — with a notice that it is deprecated — so that people can still grok older code that uses it.

The README section on the mypy plugin (marshmallow_dataclass.mypy) belongs in the section about marshmallow_dataclass.NewType. The mypy plugin's only purpose is to deal with the custom NewType.

(If we do add a DeprecationWarning to the NewType constructor, we should probably add a similar deprecation warning to uses of the mypy plugin.)

dairiki · 2024-04-29T16:10:12Z

README.md

@@ -247,28 +247,25 @@ class Sample:

 See [marshmallow's documentation about extending `Schema`](https://marshmallow.readthedocs.io/en/stable/extending.html).

-### Custom NewType declarations
+### Custom type declarations


This heading seems vague. Maybe "Custom type aliases using Annotated" or just "Custom type aliases"?

dairiki · 2024-04-29T19:56:15Z

setup.py

@@ -8,8 +8,6 @@
    "Operating System :: OS Independent",
    "License :: OSI Approved :: MIT License",
    "Programming Language :: Python",
-    "Programming Language :: Python :: 3.6",
-    "Programming Language :: Python :: 3.7",


Add classifier for Python 3.12.

dairiki · 2024-04-29T20:19:28Z

tox.ini

+commands = pytest
+extras = dev
+set_env =
+  VIRTUALENV_DISCOVERY = pyenv


This is probably better left for another PR, but since we're adding a tox.ini...

It would be nice to add combined coverage reporting for the tox run. Both to check general test coverage and to ferret out those version_info ImportError conditionals that are no longer necessary.

E.g. this works:

[tox] requires = tox>=4 virtualenv-pyenv env_list = py{38,39,310,311,312} cover-report [testenv] deps = coverage pytest commands = coverage run -p -m pytest tests extras = dev set_env = VIRTUALENV_DISCOVERY = pyenv depends = cover-report: py{38,39,310,311,312} [testenv:cover-report] skip_install = true deps = coverage commands = coverage combine coverage html coverage report

If coverage reporting is added to tox.ini, adding the following to pyproject.toml is also appropriate:

[tool.coverage.report] exclude_also = [ '^\s*\.\.\.\s*$', '^\s*pass\s*$', ]

dairiki · 2024-04-29T21:09:23Z

setup.py

@@ -18,24 +16,20 @@
 ]

 EXTRAS_REQUIRE = {
-    "enum": [
-        "marshmallow-enum; python_version < '3.7'",


I think we can remove marshmallow-enum from the requirements for the mypy pre-commit hook, too.

mvanderlee force-pushed the mvanderlee/annotated branch from 658e18f to ff35c2c Compare March 9, 2024 22:06

mvanderlee changed the title ~~Add Annotated support and therefore set minimum python version as 3.9~~ Add Annotated support Mar 10, 2024

lovasoa requested a review from dairiki March 11, 2024 07:01

dairiki mentioned this pull request Mar 11, 2024

Fix memory leak issue #258

Merged

mvanderlee mentioned this pull request Mar 11, 2024

Add Generic dataclasses #259

Draft

mvanderlee added 3 commits March 20, 2024 17:38

Add Annotated support and therefore set minimum python version as 3.9

bbd4ea8

Add support back for Python3.8

c54dff5

Fix Python 3.8 and add tox config for cross version testing

4686736

mvanderlee force-pushed the mvanderlee/annotated branch from 736833b to 4686736 Compare March 20, 2024 16:44

Fix style issues. Rebase did not trigger pre-commit.

4b8ebbf

dairiki reviewed Apr 29, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Annotated support #257

Add Annotated support #257

mvanderlee commented Mar 9, 2024

mivade commented Mar 9, 2024

mvanderlee commented Mar 20, 2024

mvanderlee commented Apr 5, 2024

dairiki left a comment

dairiki Apr 29, 2024

dairiki Apr 29, 2024

dairiki Apr 29, 2024

dairiki Apr 29, 2024

dairiki Apr 29, 2024

dairiki Apr 29, 2024

dairiki Apr 29, 2024

dairiki Apr 29, 2024

dairiki Apr 29, 2024

dairiki Apr 29, 2024

		@@ -247,28 +247,25 @@ class Sample:

		See [marshmallow's documentation about extending `Schema`](https://marshmallow.readthedocs.io/en/stable/extending.html).

		### Custom NewType declarations

Add Annotated support #257

Are you sure you want to change the base?

Add Annotated support #257

Conversation

mvanderlee commented Mar 9, 2024

mivade commented Mar 9, 2024

mvanderlee commented Mar 20, 2024

mvanderlee commented Apr 5, 2024

dairiki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment