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

Rename BaseModel.__values__ to BaseModel.__dict__ #712

Merged

Conversation

@MrMrRobat
Copy link
Contributor

commented Aug 3, 2019

Change Summary

What done

  • __values__ renamed to __dict__ all over BaseModel
  • __getattr__ and __dir__ removed as they become redundant
  • created __values__ property which will allow to use old attr, but will throw deprecation warning

What gained

  • attributes read speed increased up to 14 times (10 times average)
  • size of new models now a bit smaller
  • PyCharm will now throw warnings when trying to access attributes, that don't exist in model: #711 (comment)
  • Fix RecursionError when stepping through pydantic with PyCharm debugger #702 (comment)

Related issue number

#711 / #701

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

This comment has been minimized.

Copy link

commented Aug 3, 2019

Codecov Report

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

@@          Coverage Diff          @@
##           master   #712   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          15     15           
  Lines        2727   2721    -6     
  Branches      539    539           
=====================================
- Hits         2727   2721    -6
HISTORY.rst Outdated Show resolved Hide resolved
@dmontagu

This comment has been minimized.

Copy link
Collaborator

commented Aug 3, 2019

I noticed you are saying in the history update that removing __getattr__ is a breaking change, but I can't think of any code it could break (given object.__getattr__ would still be present.) Am I missing something? I guess removing __values__ is still a breaking change, maybe better to say that in the history

@MrMrRobat

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

Unfortunately object doesn't have __getattr__:

>>> object.__getattr__                                                                          
Traceback (most recent call last)
...
AttributeError: type object 'object' has no attribute '__getattr__'

So any some code that used BaseModel.__getattr__ will fail may work strange:

from typing import Dict, Any
from pydantic import BaseModel

class MyModel(BaseModel):
    my_values: Dict[str, Any]

    def __getattr__(self, item):
        try:
            return self.my_values[item]
        except KeyError:
            return super().__getattr__(item)

m = MyModel(my_values=dict(foo='bar'))
print(m.my_values, m.foo, sep='\n')
m.bar  # cause AttributeError
...
{'foo': 'bar'}
bar

Traceback (most recent call last):
    ...
    return self.my_values[item]
KeyError: 'bar'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
    ...
    return super().__getattr__(item)
AttributeError: 'super' object has no attribute '__getattr__'

But having thoughts about this: it will fail only if object actually doesn't have attribute (nor in __dict__ or my_values), so possibly it will not break anything at all, except for error message will be different and will point not to attribute that caused the problem, but to __getattr__

As for turning __values__ into a poperty (not removing it completely) , I can't imagine code it could break either. Any suggestions?

@dmontagu

This comment has been minimized.

Copy link
Collaborator

commented Aug 3, 2019

D'oh good catch on both points. I think what I was thinking (😅) was that getattr(obj, "key") would still work. Theoretically I guess it's possible someone could be using model.__getattr__("x") in their code, so maybe it is breaking. (This would require you to call getattr(model, "x") instead, but that should probably be used anyway).

So, maybe worth the call out, but unless we're missing something I think this seems very unlikely to affect existing code.

@MrMrRobat

This comment has been minimized.

Copy link
Contributor Author

commented Aug 3, 2019

@dmontagu, just updated #712 (comment) with 'real code' example, check it out :)

@koxudaxi

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2019

@MrMrRobat
I feel good the changed code 👍

@samuelcolvin
Copy link
Owner

left a comment

this looks great, tiny change to history then let's go. 🚀

HISTORY.rst Outdated
v0.32 (unreleased)
..................
* **breaking change**: remove ``__getattr__`` on ``BaseModel``, #712 by @MrMrRobat
* rename ``__values__`` to ``__dict__`` on ``BaseModel``, deprecation warning on use of old name,

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Aug 5, 2019

Owner

can you make all this one bullet point.

This comment has been minimized.

Copy link
@MrMrRobat

MrMrRobat Aug 5, 2019

Author Contributor

Yep, done!

ret = list(object.__dir__(self))
ret.extend(self.__values__.keys())
return ret
@property

This comment has been minimized.

Copy link
@samuelcolvin

samuelcolvin Aug 5, 2019

Owner

I guess we can leave this in for a couple of releases, I doubt it's necessary but no reason to remove it.

I assume with this change vars(model) and dir(model) still work correct, but have we checked that?

This comment has been minimized.

Copy link
@MrMrRobat

MrMrRobat Aug 5, 2019

Author Contributor

We have test_dir_fields in tests/test_main.py, so yeah, I checked that it's working

MrMrRobat added some commits Aug 5, 2019

@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Aug 5, 2019

This also fixes #702 / #701 could you add this test from there:

def test_init_inspection():
    class Foobar(BaseModel):
        x: int

        def __init__(self, **data) -> None:
            with pytest.raises(AttributeError):
                assert self.x
            super().__init__(**data)

    Foobar(x=1)

Then this is ready to merge.

@MrMrRobat

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2019

Just saw your comment here, thanks for adding it!

@samuelcolvin samuelcolvin merged commit b479b93 into samuelcolvin:master Aug 5, 2019

6 of 9 checks passed

Header rules No header rules processed
Details
Pages changed All files already 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 #20190805.5 succeeded
Details
samuelcolvin.pydantic (Job Python36) Job Python36 succeeded
Details
samuelcolvin.pydantic (Job Python37) Job Python37 succeeded
Details
@samuelcolvin

This comment has been minimized.

Copy link
Owner

commented Aug 5, 2019

Just saw your comment here, thanks for adding it!

No problem, I was working on a script to easily pull from and push to other's pull requests, so wanted to try it. :-)

Thanks so much for finding an implementing this.

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