Refactored extra types to use a single enum #352

liiight · 2019-01-07T23:29:08Z

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 · 2019-01-07T23:31:46Z

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

codecov · 2019-01-07T23:31:50Z

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 · 2019-01-08T10:01:08Z

@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 · 2019-01-08T10:04:33Z

allow_extra is True, not ignore_extra...

liiight · 2019-01-08T10:06:49Z

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 · 2019-01-08T10:09:32Z

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

They both apply on init.

liiight · 2019-01-08T10:14:16Z

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?

samuelcolvin · 2019-01-08T10:11:39Z

pydantic/main.py

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


+class ExtraAttributes(Enum):
+    DISALLOW_MUTATION = auto()


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

also, please use lower case.

sorry, ignore, allow and disallow.

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

samuelcolvin · 2019-01-08T10:12:33Z

pydantic/main.py

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


+class ExtraAttributes(Enum):


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

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

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

you mean like this?

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

Hmm, how is that exactly used?

samuelcolvin · 2019-01-08T10:14:15Z

pydantic/main.py

@@ -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:
+        """


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

Not sure where you mean

samuelcolvin · 2019-01-08T10:15:07Z

tests/test_main.py

@@ -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"""


please capture warnings and check check they're right.

samuelcolvin · 2019-01-08T10:19:58Z

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 · 2019-01-08T10:25:26Z

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 · 2019-01-08T10:27:11Z

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 · 2019-01-08T10:30:24Z

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 · 2019-01-08T10:31:18Z

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 · 2019-01-08T10:33:11Z

ignore_extra = True
allow_extra = False

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

samuelcolvin · 2019-01-08T10:37:06Z

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 · 2019-01-08T10:38:08Z

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

samuelcolvin · 2019-01-08T10:41:01Z

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.

samuelcolvin · 2019-01-09T10:41:44Z

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

liiight · 2019-01-09T11:04:44Z

sorry about the formatting

liiight · 2019-01-09T11:20:46Z

guess you took it over completely...

# Conflicts: # HISTORY.rst

samuelcolvin · 2019-01-10T11:33:31Z

docs/index.rst

-: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``)


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 · 2019-01-10T11:49:26Z

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 · 2019-01-11T10:52:56Z

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 · 2019-01-11T15:48:46Z

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 · 2019-01-11T15:52:45Z

great.

liiight · 2019-01-15T08:00:03Z

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 · 2019-01-26T18:09:39Z

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 · 2019-01-26T18:29:04Z

I'm sorry, I had a lot of stuff to catch up on due to my sickness. I'll get this done ASAP On Jan 26, 2019 20:09, "Samuel Colvin" <notifications@github.com> wrote: Hi @liiight <https://github.com/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. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#352 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AELANdK1ZInoFGwIfrprzeLS2fDIU7SGks5vHJnkgaJpZM4Z0ftB> .

samuelcolvin · 2019-02-03T17:18:30Z

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 · 2019-02-03T17:21:55Z

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

samuelcolvin · 2019-02-03T17:22:23Z

thanks.

liiight · 2019-02-04T12:45:32Z

Looks done to me @samuelcolvin

samuelcolvin

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

pydantic/main.py

Co-Authored-By: liiight <4374581+liiight@users.noreply.github.com>

samuelcolvin · 2019-02-04T13:15:27Z

awesome. Thank you very much.

liiight · 2019-02-04T13:21:49Z

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

refaactored extra types to use a single enum

de595d5

liiight added 4 commits January 8, 2019 02:06

slightly simplified

68849e5

added tests

5b744b7

fixed most stuff

c3d8aeb

docs and some simplifications

0db1294

better assert

1517b70

samuelcolvin previously requested changes Jan 8, 2019

View reviewed changes

liiight and others added 3 commits January 8, 2019 20:33

changed enum and fixed logic

1e4c962

trying to capture deprecation warning

714cf49

make format

cc0b34f

fixing tests and moving exta logic to __new__

7d76e1e

liiight added 5 commits January 10, 2019 12:45

Merge branch 'master' of https://github.com/samuelcolvin/pydantic

0381e77

# Conflicts: # HISTORY.rst

added a negative tests

2bd5857

reverted format changes

2727eb3

format

31be4e0

matched casing

f2d5d96

samuelcolvin reviewed Jan 10, 2019

View reviewed changes

samuelcolvin mentioned this pull request Jan 10, 2019

docs on how to contribute #353

Closed

renamed values

e5e2d01

liiight added 6 commits February 4, 2019 14:24

more fixes

2cb368d

forgot values change

0cb0cca

Merge

e9f1bc8

another one

fd47ccc

weird stuff

89392d1

linting issue

ab12311

samuelcolvin reviewed Feb 4, 2019

View reviewed changes

pydantic/main.py Outdated Show resolved Hide resolved

Update pydantic/main.py

1f0190e

Co-Authored-By: liiight <4374581+liiight@users.noreply.github.com>

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

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


		class ExtraAttributes(Enum):

Refactored extra types to use a single enum #352

Refactored extra types to use a single enum #352

Conversation

liiight commented Jan 7, 2019 • edited

Change Summary

Related issue number

Checklist

liiight commented Jan 7, 2019

codecov bot commented Jan 7, 2019 • edited

Codecov Report

liiight commented Jan 8, 2019

samuelcolvin commented Jan 8, 2019

liiight commented Jan 8, 2019

samuelcolvin commented Jan 8, 2019

liiight commented Jan 8, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

samuelcolvin commented Jan 8, 2019

liiight commented Jan 8, 2019

samuelcolvin commented Jan 8, 2019

liiight commented Jan 8, 2019 • edited

samuelcolvin commented Jan 8, 2019

liiight commented Jan 8, 2019

samuelcolvin commented Jan 8, 2019

liiight commented Jan 8, 2019 • edited

samuelcolvin commented Jan 8, 2019

samuelcolvin commented Jan 9, 2019

liiight commented Jan 9, 2019

liiight commented Jan 9, 2019

Choose a reason for hiding this comment

samuelcolvin commented Jan 10, 2019

samuelcolvin commented Jan 11, 2019

liiight commented Jan 11, 2019 • edited

samuelcolvin commented Jan 11, 2019

liiight commented Jan 15, 2019

samuelcolvin commented Jan 26, 2019

liiight commented Jan 26, 2019 via email

samuelcolvin commented Feb 3, 2019

liiight commented Feb 3, 2019

samuelcolvin commented Feb 3, 2019

liiight commented Feb 4, 2019

samuelcolvin left a comment

Choose a reason for hiding this comment

samuelcolvin commented Feb 4, 2019

liiight commented Feb 4, 2019

liiight commented Jan 7, 2019 •

edited

codecov bot commented Jan 7, 2019 •

edited

liiight commented Jan 8, 2019 •

edited

liiight commented Jan 8, 2019 •

edited

liiight commented Jan 11, 2019 •

edited