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 ordered schemas when Meta is defined #593

Merged
merged 1 commit into from Mar 9, 2017

Conversation

Projects
None yet
2 participants
@frol
Contributor

frol commented Mar 5, 2017

No description provided.

@frol frol force-pushed the frol:inherit-ordered-schemas branch from 79424f0 to 8340246 Mar 5, 2017

@frol

This comment has been minimized.

Contributor

frol commented Mar 5, 2017

I have reimplemented this feature in a cleaner way. Originally, I patched the meta instance right in the __new__.

@sloria

This comment has been minimized.

Member

sloria commented Mar 6, 2017

We should not be messing with Python's inheritance behavior any more than necessary. class Meta can be inherited just like a normal class.

from marshmallow import Schema

class Parent(Schema):
    class Meta:
        ordered = True


class Child(Schema):
    class Meta(Parent.Meta):
        pass

assert Child().opts.ordered is True

@sloria sloria closed this Mar 6, 2017

@frol

This comment has been minimized.

Contributor

frol commented Mar 6, 2017

Without this change we have inconsistent behavior as ordered is inferred from base classes in __new__, so I believe that either you drop that logic (that would be sad) or complete it with this change.

@frol

This comment has been minimized.

Contributor

frol commented Mar 6, 2017

I am talking about these lines:

if not ordered:
# Inherit 'ordered' option
# Warning: We loop through bases instead of MRO because we don't
# yet have access to the class object
# (i.e. can't call super before we have fields)
for base_ in bases:
if hasattr(base_, 'Meta') and hasattr(base_.Meta, 'ordered'):
ordered = base_.Meta.ordered
break
else:
ordered = False
cls_fields = _get_fields(attrs, base.FieldABC, pop=True, ordered=ordered)
klass = super(SchemaMeta, mcs).__new__(mcs, name, bases, attrs)
inherited_fields = _get_fields_by_mro(klass, base.FieldABC, ordered=ordered)
. You infer the ordered option there and since you already treat the schema as ordered, it is absolutely fine to help the SchemaOpts to set the ordered value correctly.

This behavior was introduced 2 years ago in 927704b to close #162.

BTW, if the Meta is inherited, we won't break anything as ordered is checked on the Meta:

ordered = getattr(meta, 'ordered', False)

@sloria

This comment has been minimized.

Member

sloria commented Mar 6, 2017

OK, thanks for your input. I will take a closer look at this when I have the time.

@sloria sloria reopened this Mar 6, 2017

@sloria

This comment has been minimized.

Member

sloria commented Mar 9, 2017

@frol OK, this looks like a good change. Since this is a bug fix, would you mind resending the PR to the 2.x-line branch?

@frol

This comment has been minimized.

Contributor

frol commented Mar 9, 2017

Thank you for your review! I agree it is should be in 2.x-line. I will do this shortly.

@frol frol force-pushed the frol:inherit-ordered-schemas branch from 8340246 to 3ffb74d Mar 9, 2017

@frol frol changed the base branch from dev to 2.x-line Mar 9, 2017

@frol

This comment has been minimized.

Contributor

frol commented Mar 9, 2017

Rebased and changed the target branch.

@sloria

This comment has been minimized.

Member

sloria commented Mar 9, 2017

Thanks for the quick turnaround!

@sloria sloria merged commit 6a064d9 into marshmallow-code:2.x-line Mar 9, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment