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

Work in progress: Refactor #232

Open dairiki opened 1 year ago

dairiki commented 1 year ago

I've fallen down a rabbit hole and have started work on a significant refactor of the code-base.

This refactor pulls in work from PR #172 as well as PR #214.

It is a work in progress. It's not done, however, it should theoretically be in a working state. Comments are welcome.

Summary

Issues Addressed

It fixes #230, #198, #229, #193, and #51.

It can probably address #143.

It could possibly address #146, and #56.

I haven't exhaustively checked for issues that may be addressed — there are probably more.

mvanderlee commented 7 months ago

@dairiki Any progress on this? Is there anything others can do to help?

dairiki commented 7 months ago

@dairiki Any progress on this? Is there anything others can do to help?

@mvanderlee

As you've probably noted. I stalled on this and haven't looked at it in some time.

As I was working on this, that I kept bumping up against issues with correctly supporting the .Schema class attribute that gets added to classes decorated with the @marshmallow_dataclass.dataclass decorator. I started working on a write-up of the issues encountered (see https://github.com/dairiki/marshmallow_dataclass/wiki). (The main issue is that, in some cases, the schema can not be computed (because of, e.g., forward refs in the type annotations). Deferred computation is impossible to do completely correctly — again mostly because resolving the forward type annotations is very tricky, and not completely well-defined at this point.)

Since this PR was turning into a near-complete rewrite, and since the .Schema class attribute seemed impossible to implement in a full-correct manner, the conclusion I came to is that I'd really like to drop the @marshmallow_dataclass.dataclass decorator from our API altogether. I wasn't sure how well that would go over with all y'all. I was starting to work on proposing the idea seriously (i.e.. the above-linked wiki page) but never got around to proposing the change.

So, I guess, some community (and maintainer) guidance as to the appetite for: a) a near-complete rewrite and/or b) dropping the custom dataclass decorator (and its .Schema attribute) might help motivate me to finish this.

sloria commented 7 months ago

FWIW, in my previous work as a user ofmarshmallow-dataclass, we never used the @marshmallow_dataclass.dataclass API. Instead we used class_schema to generate separate Schema classes.

@dataclass
class City:
    name: Optional[str]
    buildings: List[Building] = field(default_factory=list)

CitySchema = marshmallow_dataclass.class_schema(City)

For the cost of 1 extra LOC, you get clearer code that separates dataclass and Schema, and makes type checkers happy.

dairiki commented 7 months ago

FWIW, in my previous work as a user ofmarshmallow-dataclass, we never used the @marshmallow_dataclass.dataclass API. Instead we used class_schema to generate separate Schema classes.

@sloria Me too. And I agree.

However, based on tickets opened here and general discussion, there are quite a few who do use the magic decorator.

mvanderlee commented 6 months ago

Personally I'm okay with the removal of the decorator but certainly not with the forced use of class_schema. It'd make a lot more sense to do what pydantic did then and use inheritance with a dataclass_transform metaclass.

That being said, even pydantic supports the dataclass decorator, but with limitations. Which I think is a good compromise?

mvanderlee commented 6 months ago

Regarding forward references typing._eval_type does a great job at it. It could fail, in which case we could either let it raise an exception or wrap it in a try catch, log a warning, and don't include the failed field in the schema.

I've done something similar in the past: https://github.com/mvanderlee/starmallow/blob/master/starmallow/utils.py#L383 and pydantic does this as well. I prefer marshmallow over pydantic, but there is some cool stuff there.