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

Fix extra behaviour for multiple inheritance/mix-ins #394

Merged
merged 3 commits into from Feb 13, 2019
Merged

Fix extra behaviour for multiple inheritance/mix-ins #394

merged 3 commits into from Feb 13, 2019

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

@YaraslauZhylko YaraslauZhylko commented Feb 12, 2019

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

@codecov
Copy link

@codecov 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
Owner

@samuelcolvin samuelcolvin commented Feb 13, 2019

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

@YaraslauZhylko YaraslauZhylko commented Feb 13, 2019

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

👍

@samuelcolvin samuelcolvin merged commit d539bcd into samuelcolvin:master Feb 13, 2019
7 checks passed
@YaraslauZhylko YaraslauZhylko deleted the fix-config-extra-inheritance branch Feb 13, 2019
@YaraslauZhylko YaraslauZhylko restored the fix-config-extra-inheritance branch Feb 13, 2019
@YaraslauZhylko YaraslauZhylko deleted the fix-config-extra-inheritance branch Feb 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants