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

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

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

Effectively reverses the changes from pydantic#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
Copy link
Contributor Author

Lnaden 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
Copy link

codecov bot 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
Copy link
Contributor Author

Lnaden 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.

Copy link
Member

@samuelcolvin samuelcolvin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 pydantic:master Jul 11, 2019
@Lnaden Lnaden deleted the nested_init_and_self branch July 11, 2019 15:59
alexdrydew pushed a commit to alexdrydew/pydantic that referenced this pull request Dec 23, 2023
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