marshmallow
marshmallow copied to clipboard
Inherit options
Solves the issue documented here, where combining two schemas, each with different SCHEMA_OPTS
, need to be combined: https://github.com/marshmallow-code/marshmallow-jsonapi/issues/3
My solution automatically creates a new SchemaOpts
subclass that inherits from both parents whenever you subclass a Schema
with two parents. I could have implemented the metaclass to always do this, but this would break downstream code that might depend on the SchemaOpts
not properly inheriting. Thus, I've added a new class constructor parameter called combine_opts=True
.
An example usage for the issue above is this:
from marshmallow_jsonapi import schema as ja_schema
from marshmallow_sqlalchemy import schema as sql_schema
class ArtistSchema(sql_schema.ModelSchema, ja_schema.Schema, combine_opts=True):
class Meta:
model = models.Artist
type_ = 'artists'
I've also added a test for this parameter
This is really cool! Interested to see where it goes.
Should we also "auto-inherit" Meta
even when doing single inheritance?
class MyBaseSchema(Schema):
class Meta:
render_module = ujson
class ArtistSchema(MyBaseSchema):
class Meta: # implicitly inherits MyBaseSchema.Meta
ordered = True
I'm don't have a good sense whether this implicit inheritance matches user expectations. Do you have time to provide an evaluation of what other libraries do (e.g. Django ORM, wtforms, etc)?
Should we also "auto-inherit"
Meta
even when doing single inheritance?
The question of whether Meta
s should inherit is fairly different to the question about SchemaOpts
, because class Meta
is basically just an options object, it's not really a "class" conceptually.
I don't hate the idea of us making them inherit. But it should possibly be also locked behind a flag, because doing this by default might confuse a lot of people. Django REST Framework doesn't seem to make Meta
s inherit from each other: https://github.com/encode/django-rest-framework/blob/master/rest_framework/serializers.py#L285-L316
Basically, I don't see any harm in doing slightly unusual things here, because they'll be hidden behind a constructor flag. So it will help people who need these settings but not affect everyone else.
One motivating example I can think of for allowing Metas to inherit is setting the session
field for marshmallow-sqlalchemy
. I believe you currently have to write sqla_session = session
on each schema you declare, which is a bit redundant.
This seems like it has some risk of unpredictable behavior in the case where multiple schema classes define options classes, though. It seems like you can still end up with cases where an "earlier" base defines a parent class as the options class, or whatever, just by accident.
Perhaps the same possible issues arise when doing this explicitly, though.
In practice this may be fine for now, but the limitations of doing this sort of inheritance suggest that it might make more sense to move to something more like a class method or something that could leverage normal inheritance semantics, but probably not without a major bump.
Can you give an example of a class hierarchy where you think this might happen? The entire use case for this PR is where multiple schema classes define different options classes.
In addition, if there is a specific issue caused by this behaviour, you can just revert back to not using combine_opts=True
in the class constructor.
You can already leverage normal inheritance semantics in Marshmallow, whereby you have an options class inherit from another options class. All my PR does is give you an option to do this automatically, but if you don't want this behaviour you can instead do it manually if you want.
I'm thinking something like:
class Opts1(SchemaOpts):
pass
class Opts2(Opts1):
pass
class Schema1(Schema):
OPTIONS_CLASS = Opts1
class Schema2(Schema1):
OPTIONS_CLASS = Opts2
class Schema3(Schema1):
pass
class Schema4(Schema3, Schema2, Schema1):
pass
For Schema4
, bases ends up being (Schema3, Schema2, Schema1)
, with attributes of Schema2
overriding those on Schema1
.
But, unless I'm missing something, we'd have Schema4Opts(Opts1, Opts2)
, which inverts the order.
This is of course somewhat contrived
I suppose there might be a case where two options classes use the same keywords and thus won't play nicely together, but in general they shouldn't do that. The options classes are just bags of fields, so it's not like they have methods where it matters which one goes first. That said, if the order matters you can always a) rearrange the parent classes in the child class definition, and if that isn't satisfactory b) Manually define class Opts4(Opts1, Opts2)
instead of using combine_opts
.
I guess my point is that this is just a utility keyword that won't break backwards compatibility and it only adds utility for a very common use-case, but doesn't take away any power over inheritance.
I like the Meta auto-inheritance. It makes using a base Schema more practical.
Maybe it can be achieved in another PR.
I just opened a related RFC: #1969.