Inherit options

[multimeric]

Solves the issue documented here, where combining two schemas, each with different SCHEMA_OPTS, need to be combined: marshmallow-code/marshmallow-jsonapi#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

[antgel]

This is really cool! Interested to see where it goes.

[sloria]

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

[sloria]

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)?

[multimeric]

Should we also "auto-inherit" Meta even when doing single inheritance?

The question of whether Metas 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 Metas 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.

[multimeric]

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.

[lafrech]

I like the Meta auto-inheritance. It makes using a base Schema more practical.

Maybe it can be achieved in another PR.

Also, we could make those new features default (opt-out) behaviour in MA4.

[lafrech]

Rewrite proposal:

        if combine_opts and len(bases) > 1:
            # Create a new options class that combines all the base classes
            opts = type(
                name + "Opts", tuple({base.OPTIONS_CLASS for base in bases}), {}
            )
        else:
            opts = klass.OPTIONS_CLASS
       # Set klass.opts in __new__ rather than __init__ so that it is accessible in
       # get_declared_fields
        klass.opts = opts(meta, ordered=ordered)

[multimeric]

True, that is admittedly a lot less redundant. I'll fix it at some point

[lafrech]

I don't get the purpose of this test.

In fact, I don't understand the purpose of the Meta class attribute in both tests.

[multimeric]

The class Meta sets the values of fields on the options class. It's just normal Marshmallow behaviour.

This particular test basically just checks that using combine_opts=True works for two schemas with the same options class. Basically just checking backwards compatibility.

[taion]

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.

[multimeric]

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.

[taion]

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

[multimeric]

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.

[lafrech]

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.