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

strict value is overidable, in all case. (especially, __init__(strict=False), and Meta.strict=True) #569

Merged
merged 3 commits into from
Jan 8, 2017

Conversation

podhmo
Copy link

@podhmo podhmo commented Jan 6, 2017

Current marshmallow's implementation is not allow to overwrite strict value by strict=False, when strict value is set by Meta.

class S(Schema):
    class Meta:
        strict = True

S(strict=False).strict  # => True

this pull request is the solution for #550

@podhmo
Copy link
Author

podhmo commented Jan 6, 2017

It is a bit strange that default value of strict is None. If so, this is better, maybe.

delegated = object()

class BaseSchema(base.SchemaABC):
    .. snip
    def __init__(self, extra=None, only=(), exclude=(), prefix='', strict=delegated,
                 many=False, context=None, load_only=(), dump_only=(),
                 partial=False):

Copy link
Contributor

@maximkulkin maximkulkin left a comment

Choose a reason for hiding this comment

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

I think there is no need for custom object as default value implementation and using None instead is OK. See my inline comments though.

def test_default(self):
assert self.SchemaWithoutMeta().strict is False

def test__meta_true(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

  1. I think it is safe and more common not to use double underscores in test names.
  2. You miss two tests when you use SchemaWithoutMeta and override with init argument (both True and False).

@podhmo
Copy link
Author

podhmo commented Jan 7, 2017

Oh, yes I missed, adding two tests. And renaming test name (honestly, this is something like my individual habits, separating test method's name by double underscore for readability. But this is not strong will).

@maximkulkin
Copy link
Contributor

👍 LGTM

Just make sure to squash your 3 commits about tests into one before merging, their commit descriptions are not informative and it is completely unnecessary to have all this small fixes in master.

@podhmo
Copy link
Author

podhmo commented Jan 7, 2017

sure. rebased.

@sloria
Copy link
Member

sloria commented Jan 8, 2017

Thanks @podhmo for the PR and thanks @maximkulkin for reviewing. @podhmo : would you mind adding yourself to AUTHORS.rst? Once that's done, this is good to merge.

I consider this a bugfix, so I'll backport this to the 2.10-line branch once this is merged.

@podhmo
Copy link
Author

podhmo commented Jan 8, 2017

added.

@sloria
Copy link
Member

sloria commented Jan 8, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants