Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inherit options #1432

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions src/marshmallow/schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,12 @@ class SchemaMeta(type):
the Schema class's ``class Meta`` options.
"""

def __new__(mcs, name, bases, attrs):
def __new__(mcs, name, bases, attrs, combine_opts=False):
"""
:param combine_opts: If true, create a new SchemaOpts and Meta subclass from both parents
"""

# Collect fields and construct the class
meta = attrs.get("Meta")
ordered = getattr(meta, "ordered", False)
if not ordered:
Expand All @@ -108,10 +113,20 @@ def __new__(mcs, name, bases, attrs):
klass = super().__new__(mcs, name, bases, attrs)
inherited_fields = _get_fields_by_mro(klass, base.FieldABC, ordered=ordered)

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

# Add fields specified in the `include` class Meta option
cls_fields += list(klass.opts.include.items())

Expand Down Expand Up @@ -146,7 +161,7 @@ def get_declared_fields(
"""
return dict_cls(inherited_fields + cls_fields)

def __init__(cls, name, bases, attrs):
def __init__(cls, name, bases, attrs, combine_opts=False):
super().__init__(name, bases, attrs)
if name and cls.opts.register:
class_registry.register(name, cls)
Expand Down
77 changes: 77 additions & 0 deletions tests/test_schema.py
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
INCLUDE,
RAISE,
class_registry,
SchemaOpts,
)
from marshmallow.exceptions import (
ValidationError,
Expand Down Expand Up @@ -2766,3 +2767,79 @@ class Meta:
dumped = OSchema().dump({"foo": 42, "bar": 24})
assert isinstance(dumped, OrderedDict)
assert "bar" not in dumped


class TestCombineOpts:
def test_different_opts(self):
"""
Create two options classes, each belonging to a schema, then combine the two
schemas using inheritance. When combine_opts == True, we should have both options
being stored in self.opts. Otherwise, we should only get options defined in the
first options class stored in self.opts.
"""

class Opts1(SchemaOpts):
def __init__(self, meta, **kwargs):
super().__init__(meta, **kwargs)
self.opt_1 = getattr(meta, "opt_1", None)

class Schema1(Schema):
OPTIONS_CLASS = Opts1
Field1 = fields.String()

class Opts2(SchemaOpts):
def __init__(self, meta, **kwargs):
super().__init__(meta, **kwargs)
self.opt_2 = getattr(meta, "opt_2", None)

class Schema2(Schema):
OPTIONS_CLASS = Opts2
Field2 = fields.String()

class NotCombined(Schema1, Schema2, combine_opts=False):
class Meta:
opt_1 = True
opt_2 = True

class Combined(Schema1, Schema2, combine_opts=True):
class Meta:
opt_1 = True
opt_2 = True

# Without using combine_opts, we should only get options defined by the first
# options class
assert NotCombined().opts.opt_1
assert not hasattr(NotCombined().opts, "opt_2")

# However when we use combine_opts=True, we should get options defined by both
# options classes
assert Combined().opts.opt_1
assert Combined().opts.opt_2

def test_same_opts(self):
"""
Ensure this behaviour still works if the two parent schemas have the same opts
class
"""

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

class Opts1(SchemaOpts):
def __init__(self, meta, **kwargs):
super().__init__(meta, **kwargs)
self.opt_1 = getattr(meta, "opt_1", None)

class Schema1(Schema):
OPTIONS_CLASS = Opts1
Field1 = fields.String()

class Schema2(Schema):
OPTIONS_CLASS = Opts1
Field2 = fields.String()

class Combined(Schema1, Schema2, combine_opts=True):
class Meta:
opt_1 = True
opt_2 = True

# However when we use combine_opts=True, we should get options defined by both
# options classes
assert Combined().opts.opt_1