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

Refactored extra types to use a single enum #352

Merged
merged 28 commits into from Feb 4, 2019

Conversation

Projects
None yet
2 participants
@liiight
Copy link
Contributor

liiight commented Jan 7, 2019

Change Summary

Use a single value to describe allowing extra attributes or ignoring them, by using an enum:

class ExtraAttributes(Enum):
    DISALLOW_MUTATION = auto()
    ALWAYS_DISALLOW = auto()
    MUTATION_ONLY = auto()
    ALWAYS_ALLOW = auto()

    def allow_mutation(self):
        return self in (self.MUTATION_ONLY, self.ALWAYS_ALLOW)

    def should_ignore(self):
        return self in (self.DISALLOW_MUTATION, self.MUTATION_ONLY)

Related issue number

#351

Checklist

  • Unit tests for the changes exist
  • Tests pass on CI and coverage remains at 100%
  • Documentation reflects the changes where applicable
  • HISTORY.rst has been updated
    • if this is the first change since a release, please add a new section
    • include the issue number or this pull request number #<number>
    • include your github username @<whomever>
@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Jan 7, 2019

Still need to add new unit tests, docs and deprecation warning but I wanted to get early feedback.

@codecov

This comment has been minimized.

Copy link

codecov bot commented Jan 7, 2019

Codecov Report

Merging #352 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff          @@
##           master   #352   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          14     14           
  Lines        1910   1934   +24     
  Branches      370    375    +5     
=====================================
+ Hits         1910   1934   +24

liiight added some commits Jan 8, 2019

@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Jan 8, 2019

@samuelcolvin about the failing test:

def test_allow_extra_deprecated():
    """Will be removed in a future release"""

    class Model(BaseModel):
        a: float = ...

        class Config:
            allow_extra = True

    assert Model(a='10.2', b=12).dict() == {'a': 10.2, 'b': 12}

I'm not sure how this passed before since the default of ignore_extra is True. Shouldn't it have just ignored b?

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Jan 8, 2019

allow_extra is True, not ignore_extra...

@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Jan 8, 2019

ignore_extra: whether to ignore any extra values in input data (default: True)

If the default of ignore_extra is True and allow_extra is True, doesn't that mean that extra attributes will be ignored on init but allowed on mutation?

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Jan 8, 2019

No, see #345 which is merged but not yet deployed.

They both apply on init.

@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Jan 8, 2019

Ah, I see. So to better reflect this in my logic:

ignore_extra = True
allow_extra = False

Ignore on init, fail on mutate

ignore_extra = False
allow_extra = False

Always fail on extra

ignore_extra = False
allow_extra = True

Always allow

ignore_extra = True
allow_extra = True

Also always allow.

Correct?

If so, that's kinda weird. Are you sure you want to keep this behaviour like this?

@@ -19,14 +20,26 @@
from .validators import dict_validator


class ExtraAttributes(Enum):
DISALLOW_MUTATION = auto()

This comment has been minimized.

@samuelcolvin

samuelcolvin Jan 8, 2019

Owner

I don't think we should have different behaviour for mutation and init, Just ignore and allow should suffice.

This comment has been minimized.

@samuelcolvin

samuelcolvin Jan 8, 2019

Owner

also, please use lower case.

This comment has been minimized.

@samuelcolvin

samuelcolvin Jan 8, 2019

Owner

sorry, ignore, allow and disallow.

But since we're making a change, maybe ignored, allowed and forbidden would be clearer.

@@ -19,14 +20,26 @@
from .validators import dict_validator


class ExtraAttributes(Enum):

This comment has been minimized.

@samuelcolvin

samuelcolvin Jan 8, 2019

Owner

IntEnum please and no need for auto. 1 and 2 should be fine.

That way people aren't forced to import to use this.

This comment has been minimized.

@samuelcolvin

samuelcolvin Jan 8, 2019

Owner

On reflection, maybe better to use (str, Enum) and have values of allowed, ignored and forbidden - should be clearer for people to use.

This comment has been minimized.

@liiight

liiight Jan 8, 2019

Author Contributor

you mean like this?

class Extra(str, Enum):
    allowed = 'allowed'
    ignored = 'ignored'
    forbidden = 'forbidden'

This comment has been minimized.

@samuelcolvin

This comment has been minimized.

@liiight

liiight Jan 9, 2019

Author Contributor

Hmm, how is that exactly used?

@@ -402,28 +415,54 @@ def validate_model(model, input_data: dict, raise_exc=True): # noqa: C901 (igno
"""
validate data against a model.
"""
def _deprecated_values(config) -> bool:
"""

This comment has been minimized.

@samuelcolvin

samuelcolvin Jan 8, 2019

Owner

No need for a separate method, also move the depeciation logic to the __new__ method.

This comment has been minimized.

@liiight

liiight Jan 8, 2019

Author Contributor

Not sure where you mean

@@ -179,7 +179,9 @@ class Model(BaseModel):
assert Model().c == 0


def test_allow_extra():
def test_allow_extra_deprecated():
"""Will be removed in a future release"""

This comment has been minimized.

@samuelcolvin

samuelcolvin Jan 8, 2019

Owner

please capture warnings and check check they're right.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Jan 8, 2019

Mutation and init should have the same (or equivalent) behaviour:

  • allowed - extra fields can be included (or set) without any validation
  • ignored - all extra values/fields/attributes are ignored on init and on set
  • forbidden - any extra values/fields/attributes on init or set raises an error
@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Jan 8, 2019

Mutation and init should have the same (or equivalent) behaviour:

  • allowed - extra fields can be included (or set) without any validation
  • ignored - all extra values/fields/attributes are ignored on init and on set
  • forbidden - any extra values/fields/attributes on init or set raises an error

I like that, but this changes the default behaviour. You want it to be ignore or forbidden?

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Jan 8, 2019

Default should be ignore, I believe that's roughly the same as currently.

If there are changes to current behaviour, just include "breaking change" in history.

@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Jan 8, 2019

If we break behaviour then let's get rid of allow_extra and ignore_extra all together instead of trying to be backwards compatible, no?

I vote for this option BTW, the old behaviour was kinda confusing, which is the motivation of this. This new behaviour is way more clearer and explicit.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Jan 8, 2019

I don't think we do break current behaviour with the new setting as ignore. Can you give an example of a change in behaviour?

@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Jan 8, 2019

ignore_extra = True
allow_extra = False

This condition which would ignore on init but fail on mutate would not be supported anymore.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Jan 8, 2019

Ok, but that's a more subtle change than just removing ignore_extra and allow_extra completed.

Please keep those settings but with a deprecation warning, and add extra.

@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Jan 8, 2019

So what's the expected behaviour in that scenario? Should it correlate to ignore or forbidden?

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Jan 8, 2019

You don't need to change the current behaviour for allow_extra, just add the new setting.

If extra is ignored or forbidden __setattr__ should raise an error if you try to sett a missing attribute.

liiight and others added some commits Jan 8, 2019

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Jan 9, 2019

Probably easier at this stage to commit to your PR than try to explain what I mean in comments.

@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Jan 9, 2019

sorry about the formatting

@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Jan 9, 2019

guess you took it over completely...

liiight added some commits Jan 10, 2019

:allow_extra: whether or not to allow (and include on the model) any extra values from input data;
when ``allow_extra`` option is set to ``True`` it overrides ``ignore_extra`` (default: ``False``)
:extra: whether to ignore, allow or forbid extra attributes in model. Can use either string values of ``ignore``,
``allow`` or ``forbidden``, or use ``Extra`` enum (default is ``Extra.ignore``)

This comment has been minimized.

@samuelcolvin

samuelcolvin Jan 10, 2019

Owner

Currently, the values are ignored, allowed or forbidden.

However looking at it again ignore, allow and forbid might make more sense and would be closer to the previous usage. Choice is yours.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Jan 10, 2019

It was my intention from the start to do history and docs so I'll do that.

Thanks a lot, much appreciated.

This approach on your part left me with a bitter taste and I doubt I will contribute anything else to this project.

I quite understand that and I'm very sorry. That wasn't my intention and I agree with you that welcoming first-time contributors is key to the success of open source projects, I hope you can see that from #308, #341, #320 or #179.

This is not a criticism but rather an explanation for why I rather took over: very open-ended questions like "Not sure where you mean" and "Hmm, how is that exactly used?" frustrated me slightly and made me think that explaining what I meant was going to take a very long time, I assumed (perhaps wrongly) that you either didn't have very much experience or hadn't taken long to look into it yourself. I then decided to move the core logic into __new__ since that was going to take a lot of explaining, by the time I had then fixed tests I agree it looked rather like I'd done all the fun bits.

I also think that contributing to pydantic could be made easier with some documentation, for example, you might not have been aware of make format to fix formatting and simple make to run all tests and linting, which would have saved a few commits.

Again, sorry not to be more welcoming and thank you very much for your contribution.

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Jan 11, 2019

You've missed renaming in a few places, also linting is failing.

If it's a pain to fix, I'll do it in a couple of days.

@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Jan 11, 2019

I plan to fix it, not feeling too hot these days. If it can wait a few days, I'll do it then. Also, I plan to respond to your last comment, but like I said, I'm too sick now...

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Jan 11, 2019

great.

@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Jan 15, 2019

About your reply: I can understand now that my questions were too open ended like you said, and perhaps were better suited in an ongoing discussion in chat room instead of comments to a PR. I am by no means a new python developer (you can browse my Github profile) but I admit that I was relying more on you answering my question than really trying to dig in the code, and I also take responsibility about that.

I can also suggest that other than the suggestion you already made to improve first time contributions, that you also add docstrings, at the very least in crucial parts of the logic, like the metaclass and other non trivial sections of the code.

Anyway, I accept your apology and take responsibility in my part of the "problem".

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Jan 26, 2019

Hi @liiight, any change you could fix this? It'd be great to get it merged.

If it's a pain, I'll happily do it in a day or two.

@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Jan 26, 2019

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Feb 3, 2019

To avoid this PR having loads of conflicts, I'm waiting to merge it before merging type hints #373. Unfortunately, that means this is delaying a big improvement to pydantic.

Normally in this situation, I'd take over and get the PR finished. However given ⬆️, I'm a little 😨 😨 😨 👻 about doing that.

@liiight therefore please finish this or let me know if you'd like me to finished it.

Again thank you for your contribution and appologies for nagging. 🙏

@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Feb 3, 2019

I apologize for the delays and I oblige to finish it within the next 24 hours

@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Feb 3, 2019

thanks.

liiight added some commits Feb 4, 2019

@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Feb 4, 2019

Looks done to me @samuelcolvin

@samuelcolvin
Copy link
Owner

samuelcolvin left a comment

Tiny change you can just accept, then I'll merge.

Show resolved Hide resolved pydantic/main.py Outdated
Update pydantic/main.py
Co-Authored-By: liiight <4374581+liiight@users.noreply.github.com>

@samuelcolvin samuelcolvin merged commit 9900c7f into samuelcolvin:master Feb 4, 2019

3 checks passed

codecov/project 100% (+0%) compared to 61e7589
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
@samuelcolvin

This comment has been minimized.

Copy link
Owner

samuelcolvin commented Feb 4, 2019

awesome. Thank you very much.

@liiight

This comment has been minimized.

Copy link
Contributor Author

liiight commented Feb 4, 2019

Sure. Let's hope the next PR wont be this difficult 😂

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