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

Add Generic dataclasses #259

Open mvanderlee opened 5 months ago

mvanderlee commented 5 months ago

Fixes: #230, possbily #267, #269

Building on the fantastic work of @onursatici and @dairiki in PRs #172 and #232

Currently broken on py3.6, but once #257 is merged I'll rebase and thus remove py3.6 and py3.7 support.

I've tested this with tox on python versions 38, 39, 310, 311, 312

Spoonrad commented 2 months ago

Exciting PR, I've been waiting a long time for generic support in dataclasses :) Do you have any update please?

mvanderlee commented 2 months ago

@dairiki @lovasoa This is ready for review.

@Spoonrad Consider this the update

mvanderlee commented 2 months ago

I came up with 1 additional usecase, with 3 solutions that are somewhat related to Annotated and Generics. I have existing NewTypes like this:

DelimitedListStr = NewType('DelimitedListStr', list[str], field=lambda *args, **kwargs: DelimitedList(mf.String(), *args, **kwargs))

I came up with these 3 solutions for this. One uses generics, the other doesn't.

  1. Using Generics:
    DelimitedListStr = Annotated[List[str], DelimitedList[mf.String]]

Edit: In the following comments we've decided against supporting callables like options 2 and 3.

  1. Using partial:
    DelimitedListStr = Annotated[List[str], functools.partial(DelimitedList, mf.String)]
  2. Using callable:
    DelimitedListStr = Annotated[List[str], lambda *args, **kwargs: DelimitedList(mf.String, *args, **kwargs)]

Support for each has been added, including unittests. I also broke out the generic handling into it's own file.

dairiki commented 2 months ago
  1. Using partial:
DelimitedListStr = Annotated[List[str], functools.partial(DelimitedList, mf.String)]
  1. Using callable:
DelimitedListStr = Annotated[List[str], lambda *args, **kwargs: DelimitedList(mf.String, *args, **kwargs)]

As API choices, these make me nervous.

Recognizing generic callables as (potential) schema field annotations, and then testing them with a zero-arg call, as is done here

https://github.com/lovasoa/marshmallow_dataclass/blob/4531c35cf5f1b50e1dbc793395377a380aae4bf6/marshmallow_dataclass/__init__.py#L1090-L1097

seems both confusing and dangerous.


Can the same functionality be obtained by creating a custom Marshmallow Field subclass? E.g. something like:

class DelimitedStrListField(DelimitedList):
    def __init__(self, *args, **kwargs):
        super().__init__(mf.String, *args, **kwargs)

DelimitedListStr = Annotated[List[str], DelimitedStrListField]
mvanderlee commented 2 months ago

Right.. We don't yet know if the annotation is supposed to be a marshmallow Field compatible callable. So we could be calling anything including eraseDiskDrive().

The class approach certainly works, I was trying to use a one liner. But the partial approach works for that, and is safer from an API perspective. We could introduce a special callable wrapper, so the user can use a callable but has to explicitly tell us about it.

DelimitedListStr = Annotated[List[str], MaFieldCallable(lambda *args, **kwargs: DelimitedList(mf.String, *args, **kwargs))]

but I don't see the value in that. over using either the partial or custom class approach.

dairiki commented 2 months ago

The class approach certainly works, I was trying to use a one liner. But the partial approach works for that, and is safer from an API perspective.

I hadn't noticed that you explicitly check for a partial applied to a Field (here). That is safe.

I still think it adds unnecessary clutter to the API.

Also, allowing a partial but not an equivalent arbitrary callable seems potentially confusing. It doesn't add any capability to the API, other than to enable specific one-liners.

We could introduce a special callable wrapper, so the user can use a callable but has to explicitly tell us about it.

A value of that would be that it is more explicitly readable: it makes the intent of the annotation more clear to the unindoctrinated reader.


I'm in favor of stopping at recognizing annotations that are either subclasses or instances of Field. That's an understandable, simple-to-explain API that's fairly self-evident, even to those unfamiliar with it.

I don't really see the extra three lines required to define a custom Field subclass as too verbose. (As well, doing so provides a good place to add a docstring, if that would help clarify the intent.)

mvanderlee commented 2 months ago

I'll switch to using the generic approach myself. So I'm fine with removing the partial approach. I mostly wanted to support an undocumented feature that works with NewType.

allowing a partial but not an equivalent arbitrary callable seems potentially confusing.

Agreed. I did investigate further but couldn't find a satisfactor way to get typehints of lambdas, even when using the Callable typehint.

While I agree that 3 lines isn't bad, given that I have multiple one lines currently, each would take up 3 + 2 blank lines (style guide) So instead of 5 lines, it'd be 25 lines. Which is harder to read at a glance.

But again, generics fixes my usecase and I can't find a satisfactory answer to the valid concerns being brought up.

mvanderlee commented 1 month ago

Bump.

Syntaf commented 1 month ago

+1 to this PR, love this @mvanderlee -- we were just recently dealing with this issue and ended up creating our own bespoke field that handled this, but if this gets merged in we'd love to bump our version and use this.

lovasoa commented 1 month ago

@mvanderlee I'm really sorry, I don't have a lot of bandwidth.

@dairiki , maybe you can review this?

dairiki commented 1 month ago

@mvanderlee I'm really sorry, I don't have a lot of bandwidth.

@dairiki , maybe you can review this?

I apologize as well. For the past month I've been mostly out of town and swamped with other concerns and activities. I'm home again now, but still trying to catch up. I will try to find time for this in the next couple of weeks (but no guarantees).