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

Re-enable nested model init calls while still allowing self #644

Merged
merged 4 commits into from Jul 11, 2019

Conversation

@Lnaden
Copy link
Contributor

commented Jul 8, 2019

Change Summary

This commit enables nested model __init__ statements to be executed
while still allowing self as an argument.

Effectively reverses the changes from #632 while still enabling the
feature it implemented. In theory, there will still be a collision if
someone ever tried to use pydantic_base_model/settings_init as an arg,
but I don't know how to engineer a case where a collision would never
happen, I'm not sure there is one.

This commit also added a test for both BaseModel and BaseSettings for
both the self-as-a-parameter and the nested __init__ features since
BaseSettings now has the same issue as BaseModel since it invoked
an __init__ with self.

I have added a comment under the __init__ for both BaseModel and
BaseSetting since not having self as the first arg is such a
rarity within Python that it will likely confuse future developers who
encounter it.

The actual name of the variable referencing the class itself can be
up for debate.

cc and credit @dgasmith for solution idea

Related issue number

#632 and #629

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>
Re-enable nested model init calls while still allowing self
This commit enables nested model `__init__` statements to be executed
while still allowing `self` as an argument.

Effectively reverses the changes from #632 while still enabling the
feature it implemented. In theory, there will still be a collision if
someone ever tried to use `pydantic_base_model/settings_init` as an arg,
but I don't know how to engineer a case where a collision would *never*
happen, I'm not sure there is one.

This commit also added a test for both BaseModel` and `BaseSettings` for
both the `self`-as-a-parameter and the nested `__init__` features since
`BaseSettings` now has the same issue as `BaseModel` since it invoked
an `__init__` with self.

I have added a comment under the `__init__` for both `BaseModel` and
`BaseSetting` since not having `self` as the first arg is such a
rarity within Python that it will likely confuse future developers who
encounter it.

The actual name of the variable referencing the class itself can be
up for debate.
@Lnaden

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

Thinking about this some more, this does work in the case where the subclass which overwrite's the __init__ uses self as a the first arg and still has self as a parameter. That may just be a corner case that should be excluded though since again, no mater what we do, I don't think there is a way to block a collision from the first arg of __init__ and still enable the calls to it.

It might be possible to write a corner case for when someone passes in self as a parameter to track it internally as something else, but that sounds like way more complex, highly-conditional engineering than should be considered.

@codecov

This comment has been minimized.

Copy link

commented Jul 8, 2019

Codecov Report

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

@@          Coverage Diff          @@
##           master   #644   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines        2655   2651    -4     
  Branches      524    524           
=====================================
- Hits         2655   2651    -4
@Lnaden

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2019

There is also the option of simply not supporting fields called "self" in the first place as the solution to avoiding that is very non-Pythonic and likely confusing to Python users in the first place (i.e. explicitly reversing #629). I'm also going to continue the discussion on #629 about this though to keep everything in one place.

@samuelcolvin
Copy link
Owner

left a comment

Thanks for the contribution, mostly looks good. Dumb of me not to think of this before.

Just a few things to change.

HISTORY.rst Outdated Show resolved Hide resolved
HISTORY.rst Outdated Show resolved Hide resolved
pydantic/env_settings.py Outdated Show resolved Hide resolved
pydantic/main.py Outdated Show resolved Hide resolved
pydantic/env_settings.py Outdated Show resolved Hide resolved

@samuelcolvin samuelcolvin merged commit 61c8ca2 into samuelcolvin:master Jul 11, 2019

4 of 7 checks passed

Header rules No header rules processed
Details
Pages changed 2 new files uploaded
Details
Redirect rules No redirect rules processed
Details
Mixed content No mixed content detected
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
deploy/netlify Deploy preview ready!
Details
samuelcolvin.pydantic Build #20190709.1 succeeded
Details

@Lnaden Lnaden deleted the Lnaden:nested_init_and_self branch Jul 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.