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 takes_self to Factory and @_CountingAttr.default #189

Merged
merged 2 commits into from May 16, 2017

Conversation

Projects
None yet
4 participants
@hynek
Member

hynek commented May 13, 2017

Now this is a biggie but I believe it’s worth it for 17.1. There should be no performance penalty but it’s a feature that people have been yearning for since day one. :)

Let me know what you think, contains some boy scouting. Most notably the completely hosted markup in api.rst around evolve. Also WTF was with local dunder variables in methods? I suppose copy pasta?

This enables the following:

@attr.s
class C:
    x = attr.ib(default=1)
    y = attr.ib(default=attr.Factory(lambda self: self.x + 1, takes_self=True))
    z = attr.ib()

    @z.default
    def whatever(self):
        return self.y + 1

Now calling C() gives you C(x=1, y=2, z=3).

Fixes #165

@hynek hynek requested a review from Tinche May 13, 2017

@codecov

This comment has been minimized.

codecov bot commented May 13, 2017

Codecov Report

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

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #189   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           9      9           
  Lines         560    579   +19     
  Branches      124    126    +2     
=====================================
+ Hits          560    579   +19
Impacted Files Coverage Δ
src/attr/exceptions.py 100% <100%> (ø) ⬆️
src/attr/_make.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fbe0bd5...570de2b. Read the comment docs.

@bluetech

This comment has been minimized.

bluetech commented May 14, 2017

This is somewhat evil, but have you considered counting the number of arguments to the factory function, instead of taking takes_self explicitly? Should be possible with something like takes_self = len(inspect.signature(factory_fn).parameters) == 1. Note: I do not endorse this :)

@hynek

This comment has been minimized.

Member

hynek commented May 14, 2017

Not only did I consider it, I've even implemented it. :)

But it seemed too magicy with too many weird edge cases. It's always possible to add magic but it's impossible to remove it. And I reckon the decorator approach will be the main one anyways...

@Tinche

This comment has been minimized.

Member

Tinche commented May 14, 2017

OK, I'm starting looking at this. Let's talk API first :)

The Factory with takes_self is a little verbose IMO. Would you consider having another class which implies takes_self?

@attr.s
class A:
    a = attr.ib()
    b = attr.ib(default=attr.SelfFactory(lambda self: self.a + 1))
    c = attr.ib(default=attr.Factory(lambda self: self.a + 1, takes_self=True)  # for comparison

Another suggestion. I've been mulling over #178 with spare brain cycles. I've come to realize the wordy

@attr.s
class Cache(object):
    _stored = attr.ib(default=attr.Factory(list))
    _by_name = attr.ib(default=attr.Factory(dict))
    _by_id = attr.ib(default=attr.Factory(dict))

could be rewritten like this:

from attr import Factory as call

@attr.s
class Cache(object):
    _stored = attr.ib(default=call(list))
    _by_name = attr.ib(default=call(dict))
    _by_id = attr.ib(default=call(dict))

To me this reads much better and is possible today. Honestly I think I would prefer this rather than allow both new and default in attr.ib().

What if we actually implemented call on top of attr.Factory, and put some magic into it? So if you want, just use attr.Factory directly, but 99% of usages would be call.

from attr import call

@attr.s
class A:
    a = attr.ib()
    b = attr.ib(default=attr.SelfFactory(lambda self: self.a + 1))
    c = attr.ib(default=attr.Factory(lambda self: self.a + 1, takes_self=True)
    d = attr.ib(default=call(lambda self: self.a + 1))
@hynek

This comment has been minimized.

Member

hynek commented May 14, 2017

I like the idea of a smart call. It solves two problems at once and doesn't risk any backward incompatibilities.

But that should be a separate PR, right? Let's merge this first and implement call on top of it. I promise there won't be 17.1 w/o call. ;)

@hynek

This comment has been minimized.

Member

hynek commented May 14, 2017

Hm, maybe it shouldn't be @x.default but @x.factory Tho?

@Tinche

This comment has been minimized.

Member

Tinche commented May 14, 2017

👍 to separate PR.

As for @a.default vs @a.factory, I think I prefer default since:

  • it's directly analogous to attr.ib(default=...)
  • you can't have both attr.ib(default=...) and @a.default
  • there is no attr.ib(factory=...).

You can't really decorate a value so I'm OK with the fact the decorator is applied to a method and the attr.ib argument to a value.

I'll take a look at the implementation first thing I have the chance. You said you wanted this before PyCon, that's on Wednesday?

@hynek

This comment has been minimized.

Member

hynek commented May 14, 2017

Yeah I travel on Wednesday but accordingly my Tuesday is rather dense so sooner would be better. :)

@Tinche

This comment has been minimized.

Member

Tinche commented May 14, 2017

Given the following test class:

import attr


@attr.s(slots=True)
class A:
    a = attr.ib(default=0)
    b = attr.ib(default=attr.Factory(list))
    c = attr.ib(default=attr.Factory(lambda self: self.b, takes_self=True))
    d = attr.ib()

    @d.default
    def _d_default(self):
        return self.a + 1

this init gets generated:

def __init__(self, a=attr_dict['a'].default, b=NOTHING, c=NOTHING, d=NOTHING):
    self.a = a
    if b is not NOTHING:
        self.b = b
    else:
        self.b = attr_dict['b'].default.factory()
    if c is not NOTHING:
        self.c = c
    else:
        self.c = attr_dict['c'].default.factory(self)
    if d is not NOTHING:
        self.d = d
    else:
        self.d = attr_dict['d'].default.factory(self)

which is fine, nothing wrong with it. However, we can speed it up, as always, by shifting work. This init does a dict lookup (on attr_dict), then an instance look up on that, then a method call. This is unnecessary. If we just inject the factory into the globals and generate an init like this:

def __init__(self, a=attr_dict['a'].default, b=NOTHING, c=NOTHING, d=NOTHING):
    self.a = a
    if b is not NOTHING:
        self.b = b
    else:
        self.b = __attr_factory_b()
    if c is not NOTHING:
        self.c = c
    else:
        self.c = __attr_factory_c(self)
    if d is not NOTHING:
        self.d = d
    else:
        self.d = __attr_factory_d(self)

my benchmark goes from 1.69 us +- 0.05 us to 1.38 us +- 0.07 us, which is significant (~22% speedup).
Here's a patch, apply with git apply: changes.txt

There, now the ball is in your court :p

@hynek

This comment has been minimized.

Member

hynek commented May 15, 2017

Aaaand it’s back to you fine sir!

@Tinche

Tinche approved these changes May 16, 2017

@hynek hynek merged commit a328e67 into master May 16, 2017

4 checks passed

codecov/patch 100% of diff hit (target 100%)
Details
codecov/project 100% (+0%) compared to fbe0bd5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@hynek hynek deleted the default-decorator branch May 16, 2017

@exarkun

This comment has been minimized.

exarkun commented May 18, 2017

The renaming of default to _default in Attribute's initializer broke txaws, fwiw.

@exarkun

This comment has been minimized.

exarkun commented May 18, 2017

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