Skip to content

Conversation

YaraslauZhylko
Copy link
Contributor

@YaraslauZhylko YaraslauZhylko commented Feb 12, 2019

Change Summary

New Config.extra supports both Enum and str values. And set_extra method always tries to convert the value to make sure it is an Enum.

But this leads to extra being re-constructed/re-assigned each time BaseModel is inherited from (even it it was not set in child classes Config) thus breaking extra inheritance when using mix-ins/multiple inheritance.

So Config.extra conversion/re-assignment was limited only to cases when extra is not an instance of Extra enum.

Rationale:

  • If extra is an Extra enum value, then it was either:
    • inherited from one of the base Configs (BaseConfig has extra = Extra.ignore), and does not need to be re-assigned to the topmost MRO class.
    • was set directly in the topmost MRO class, and does not need to be re-assigned either.
  • if extra in not an Extra enum value, than it was certainly set in the topmost MRO class (all base classes have already done the conversion), and is safe to be converted and re-assigned without messing MRO.

Related issue number

#392

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>

@YaraslauZhylko
Copy link
Contributor Author

@samuelcolvin Do we need a new version in HISTORY.rst?

@codecov
Copy link

codecov bot commented Feb 12, 2019

Codecov Report

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

@@          Coverage Diff          @@
##           master   #394   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          14     14           
  Lines        2059   2060    +1     
  Branches      419    420    +1     
=====================================
+ Hits         2059   2060    +1

@samuelcolvin
Copy link
Member

looks good to me. Good catch, thank you very much.

I try and merge PRs in the order they were created to make conflicts as fair as possible, so I'll try and get other PRs merged before this.

@YaraslauZhylko
Copy link
Contributor Author

I'll try and get other PRs merged before this.

👍

@samuelcolvin samuelcolvin merged commit d539bcd into pydantic:master Feb 13, 2019
@YaraslauZhylko YaraslauZhylko deleted the fix-config-extra-inheritance branch February 13, 2019 19:50
@YaraslauZhylko YaraslauZhylko restored the fix-config-extra-inheritance branch February 13, 2019 20:03
@YaraslauZhylko YaraslauZhylko deleted the fix-config-extra-inheritance branch February 14, 2019 22:30
alexdrydew pushed a commit to alexdrydew/pydantic that referenced this pull request Dec 23, 2023
* Add min-length support for GeneratorSchema

* Add tests for actual generators

* Remove typing import from tests

* tweak tests

* tweak tests, more

---------

Co-authored-by: Samuel Colvin <s@muelcolvin.com>
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.

2 participants