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

An attempt at attribute hooks #3

Closed
wants to merge 2 commits into from
Closed

An attempt at attribute hooks #3

wants to merge 2 commits into from

Conversation

lvh
Copy link
Owner

@lvh lvh commented Aug 8, 2014

No description provided.

@coveralls
Copy link

Coverage Status

Coverage remained the same when pulling eff9091 on attr-hooks into 076d4d5 on master.

@lvh
Copy link
Owner Author

lvh commented Aug 8, 2014

I'm thinking that the inspect.getboundargs hacks are sufficiently commonly necessary but also obscure that maybe they should be added to thimble as well; txkazoo will cetainly want to use them.

>>> def hook(thimble, attr, val):
... @wraps(val)
... def run_callback_in_reactor_thread_wrapper(*a, **kw):
... callargs = getcallargs(val, *a, **kw)

Choose a reason for hiding this comment

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

This code seems to be quite complicated to be in README. I wonder if you can show a simpler one to easily grasp the idea.

@manishtomar
Copy link

I was initially thinking this feature is not required / complicated but I cannot think of an alternative.

@manishtomar
Copy link

The only alternative would be like

class KazooClientWrapper(object):
    def __init__(self, client):
        self.client = client

    def add_listener(self, listener):
        """ Specific impl """
        # do stuff 
        return self.client.add_listener()

    def __getattr__(self, name):
        return getattr(self.client, name)

txkazooclient = Thimble(reactor, pool, KazooClientWrapper(KazooClient()), ['get'])

which I am not sure if is a good idea.

@lvh
Copy link
Owner Author

lvh commented Aug 11, 2014

Nah, I agree, that looks pretty good. I was trying to get a general API out for that, but the thing that came out that covers all the edge cases is just too darn ugly. I'm going to try and do the wrapper thing instead. Thanks @manisht!

@lvh lvh closed this Aug 11, 2014
@manishtomar
Copy link

For future communications: my github handle is @manishtomar. Not @manisht :)

@lvh
Copy link
Owner Author

lvh commented Aug 12, 2014

Oops, sorry :( Confused with IRC nick.

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.

None yet

3 participants