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

Add ability for ``__post_init__`` method to be defined. #111

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
7 participants
@tbeadle
Contributor

tbeadle commented Nov 11, 2016

If this method is defined for a class, it will get executed at the end
of __init__. Previously, there was no way to extend what was
done during __init__ when using @attr.s.

@@ -680,6 +680,10 @@ def fmt_setter_with_converter(attr_name, value_var):
a.name))
names_for_globals[val_name] = a.validator
names_for_globals[attr_name] = a
lines.extend([
"func = getattr(self, '__post_init__', None)",

This comment has been minimized.

@hynek

hynek Nov 11, 2016

Member

This is approach means that attrs gets universally slower for everyone. attrs is explicitly static in its approach. We need to check for the existence of the method when assembling the class and call it unconditionally if it’s present.

@tbeadle tbeadle force-pushed the tbeadle:post_init branch from 6106e0d to 1ac1cdd Nov 11, 2016

@codecov-io

This comment has been minimized.

codecov-io commented Nov 11, 2016

Current coverage is 100% (diff: 100%)

Merging #111 into master will not change coverage

@@           master   #111   diff @@
====================================
  Files           8      8          
  Lines         466    468     +2   
  Methods         0      0          
  Messages        0      0          
  Branches      105    106     +1   
====================================
+ Hits          466    468     +2   
  Misses          0      0          
  Partials        0      0          

Powered by Codecov. Last update cbfb229...7724d31

script, globs = _attrs_to_script(
attrs,
frozen,
post_init=hasattr(cls, '__post_init__')

This comment has been minimized.

@hynek

hynek Nov 11, 2016

Member

No hasattr ever please. This lends itself to getattr(cls, "__post_init__", False).

@tbeadle tbeadle force-pushed the tbeadle:post_init branch from 1ac1cdd to 0f6636b Nov 11, 2016

@hynek

I have added a bunch of comments many of which arguably should have been covered by the contribution guide so I’ve added it to my own todo list to fix that.

If you have problems with Hypothesis, offering candy to @Tinche always helped for me. ;)

Thank you for your contribution! And please remember to ping the ticket once you’re done.

@@ -540,7 +544,7 @@ def validate(inst):
a.validator(inst, a, getattr(inst, a.name))
def _attrs_to_script(attrs, frozen):
def _attrs_to_script(attrs, frozen, post_init=False):

This comment has been minimized.

@hynek

hynek Nov 13, 2016

Member

Any reason why this has a default value? I find them indicative of problems if used outside public APIs.

@@ -680,6 +684,8 @@ def fmt_setter_with_converter(attr_name, value_var):
a.name))
names_for_globals[val_name] = a.validator
names_for_globals[attr_name] = a
if post_init:
lines.append("self.__post_init__()")

This comment has been minimized.

@hynek

hynek Nov 13, 2016

Member

My guts tell me the post init should be called before validating the object. But I’d love input from people who actually care about this feature. Summoning all the bikeshed artists from #68: @glyph @Insoleet @wearpants @Tinche @wbolster @mathieulongtin @ambv

Speak now or forever hold your peace.

This comment has been minimized.

@mathieulongtin

mathieulongtin Nov 13, 2016

I'd say post validation. I'd expect __post_init__ to be the very last thing init calls. Beside, we can call attr.validate.

This comment has been minimized.

@Tinche

Tinche Nov 13, 2016

Member

I'm voting post validation/conversion. The name __post_init__ itself suggests it's called after the generated __init__, so I'd expect fields to be validated, converted and set. Having it called post-conversion lowers the amount of possible states we can expect the object to be in.

This comment has been minimized.

@hynek

hynek Nov 14, 2016

Member

So be it.

We’ll have to add the pass-self-to-factories thing then because otherwise there’s no way to validate dependent attributes.

This comment has been minimized.

@glyph

glyph Nov 14, 2016

Contributor

The other way to validate dependent attributes would be for __post_init__ to raise an exception, no?

This comment has been minimized.

@hynek

hynek Nov 14, 2016

Member

Sure but I like the idea of separating the factory process from the validation process.

@@ -302,6 +302,22 @@ class D(object):
assert C.D.__name__ == "D"
assert C.D.__qualname__ == C.__qualname__ + ".D"
def test_post_init(self):

This comment has been minimized.

@hynek

hynek Nov 13, 2016

Member

While this test is fine as it is, we make sure that new features work well with the rest of attrs by making them part of our Hypothesis testing strategies: https://github.com/hynek/attrs/blob/master/tests/utils.py#L148-L171

And while squinting at this code I also realized that we should add post_init support to make_class so we have API parity! It should take a callable for argument and attach it as a method to the created class before applying attrs magic to it in https://github.com/hynek/attrs/blob/master/src/attr/_make.py#L816

This comment has been minimized.

@tbeadle

tbeadle Nov 14, 2016

Contributor

make_class already supports it. A __post_init__ key can be defined with the dictionary passed in to make_class:

>>> import attr
>>> def foo(self):
...     print('in foo')
...
>>> C1 = attr.make_class('C1', {'x': attr.ib(default=42), '__post_init__': foo})
>>> c = C1()
in foo

This comment has been minimized.

@hynek

hynek Nov 17, 2016

Member

Fair enough, please just add it to our testing strategy then.

This comment has been minimized.

@hynek

hynek Nov 17, 2016

Member

oh wait you did never mind :) I’ll have another deeper look on the weekend

@@ -551,6 +551,25 @@ You can still have power over the attributes if you pass a dictionary of name: `
>>> i.y
[]
Sometimes, you want to have your class's ``__init__`` method do more than just

This comment has been minimized.

@hynek

hynek Nov 13, 2016

Member

Semantic new lines pls.

@@ -13,6 +13,9 @@ Changes:
- Don't overwrite ``__name__`` with ``__qualname__`` for ``attr.s(slots=True)`` classes.
`#99 <https://github.com/hynek/attrs/issues/99>`_
- Allow for a ``__post_init__`` method that, if defined, will get executed at

This comment has been minimized.

@hynek

hynek Nov 13, 2016

Member

We use semantic new lines in prose. Simply make each sentence a new line please.

Also please add a link to this PR like in the entry before.

self2.z = self2.x + self2.y
c = C(x=10, y=20)
assert hasattr(c, 'z')

This comment has been minimized.

@hynek

hynek Nov 13, 2016

Member

no hasattr

c = C(x=10, y=20)
assert hasattr(c, 'z')
assert c.z == 30

This comment has been minimized.

@hynek

hynek Nov 13, 2016

Member

expected == actual please

Add ability for ``__post_init__`` method to be defined.
If this method is defined for a class, it will get executed at the end
of ``__init__``.  Previously, there was no way to extend what was
done during ``__init__`` when using ``@attr.s``.

@tbeadle tbeadle force-pushed the tbeadle:post_init branch from 0f6636b to 7724d31 Nov 14, 2016

@wearpants

This comment has been minimized.

wearpants commented Nov 15, 2016

Late to the party, but yes absolutely post conversion

@hynek hynek added this to the 16.3.0 milestone Nov 19, 2016

@hynek hynek closed this in 1fbaa20 Nov 20, 2016

@hynek

This comment has been minimized.

Member

hynek commented Nov 20, 2016

I’ve fixed it up so we don’t have ping-pong over minutiae anymore, thank you very much for your contribution!

I’ve decided to rename __post_init__ to __attrs_post_init__ for consistency with __attrs_attrs__ and to be explicit about it’s heritage.

If someone has good arguments against it, feel free to open a new issue before 16.3.0 is out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment