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

Implemented LazyObject class, which provides a 'lazy' class method to create instances with lazy initialization. Th… #665

Closed
wants to merge 1 commit into from

Conversation

@crunk1
Copy link
Contributor

crunk1 commented Dec 22, 2015

…e lazy class method returns a Proxy object that subclasses the target object class. Upon accessing the proxy object in any way, the object is initialized.

Modified Google Compute Engine License objects, GCELicense, to be such a lazy object. This addresses https://issues.apache.org/jira/browse/LIBCLOUD-786.

Tests/Verification:
tox -e lint
python setup.py test
Added test/common/test_base.py which has LazyObjectTest

@crunk1
Copy link
Contributor Author

crunk1 commented Dec 22, 2015

@crunk1 crunk1 changed the title Implemented LazyObject class, which provides a .lazy class method. Th… Implemented LazyObject class, which provides a 'lazy' class method to create instances with lazy initialization. Th… Dec 22, 2015
self._request()

def _request(self):
# TODO(crunkleton@google.com): create new connection? or make

This comment has been minimized.

@tonybaloney

tonybaloney Dec 22, 2015 Contributor

@crunk1 so what is the intention with the todo? to raise a separate PR later

This comment has been minimized.

@crunk1

crunk1 Dec 22, 2015 Author Contributor

Yes, I need to look into this later.

@tonybaloney
Copy link
Contributor

tonybaloney commented Dec 28, 2015

LGTM +1

pass

def __getattribute__(self, attr):
lazy_obj = object.__getattribute__(self, 'lazy_obj')

This comment has been minimized.

@jimbobhickville

jimbobhickville Dec 28, 2015 Contributor

Should you prefix the lazy_obj attr with an underscore (_lazy_obj) to indicate it's intended to be a private attr?

This comment has been minimized.

@jimbobhickville

jimbobhickville Dec 28, 2015 Contributor

Also, I believe if you override __getattr__ instead of __getattribute__ then you can just check the attribute value without having to invoke __getattribute__ directly like this:

if self.lazy_obj is None:

But maybe there's something subtle here I'm missing?

This comment has been minimized.

@crunk1

crunk1 Dec 29, 2015 Author Contributor

Changed LazyObject.Proxy.lazy_obj to LazyObject.Proxy._lazy_obj.

Regarding the __getattr__ vs __getattribute__ discussion, see http://stackoverflow.com/a/3278104/1436495.
My considerations:

  • __getattr__ is only called if attribute resolution fails by normal means
  • __getattribute__ is ALWAYS called (even on bound methods); easier to handle given my next point
  • the Proxy metaclass inherits from lazy_cls (I want Proxy to be as transparent as possible as a wrapper; I want the returned Proxy instance to have the class info for lazy_cls), meaning that Proxy inherits all methods and class attributes of lazy_cls. Therefore, weird things happen when you call methods or use class-level attributes intended for the lazy object instead of the Proxy.
if lazy_obj is None:
lazy_obj = lazy_cls(*lazy_init_args, **lazy_init_kwargs)
object.__setattr__(self, 'lazy_obj', lazy_obj)
return lazy_obj.__getattribute__(attr)

This comment has been minimized.

@jimbobhickville

jimbobhickville Dec 28, 2015 Contributor

I believe it's generally preferable to use the getattr function than invoking __getattribute__ directly on a foreign object. return getattr(lazy_obj, attr)

This comment has been minimized.

@crunk1

crunk1 Dec 29, 2015 Author Contributor

True, thanks for the heads up! I got a little object.__getattribute__ happy.

if lazy_obj is None:
lazy_obj = lazy_cls(*lazy_init_args, **lazy_init_kwargs)
object.__setattr__(self, 'lazy_obj', lazy_obj)
lazy_obj.__setattr__(attr, value)

This comment has been minimized.

@jimbobhickville

jimbobhickville Dec 28, 2015 Contributor

Same advice about setattr(lazy_obj, attr, value) as above.

This comment has been minimized.

@crunk1

crunk1 Dec 29, 2015 Author Contributor

Done

…e lazy class method returns a Proxy object that subclasses the target object class. Upon accessing the proxy object in any way, the object is initialized.

Modified Google Compute Engine License objects, GCELicense, to be such a lazy object. This addresses https://issues.apache.org/jira/browse/LIBCLOUD-786.

Tests/Verification:
  tox -e lint
  python setup.py test
  Added test/common/test_base.py which has LazyObjectTest
pass

def __getattribute__(self, attr):
lazy_obj = object.__getattribute__(self, '_get_lazy_obj')()

This comment has been minimized.

@jimbobhickville

jimbobhickville Dec 29, 2015 Contributor

Wrapping it in a method was nice, but I don't think you need to use __getattribute__ to call that method since it's defined in the same class here, so any invocation of this method is guaranteed to have self._get_lazy_obj() available to call.

This comment has been minimized.

@crunk1

crunk1 Dec 29, 2015 Author Contributor

Since the Proxy class has __getattribute__ defined, self._get_lazy_obj() will first call Proxy.__getattribute__(self, '_get_lazy_obj') to get the bound method. That leads to an infinite recursion loop. See an example here: https://repl.it/Bavg/1.

This comment has been minimized.

@jimbobhickville

jimbobhickville Dec 29, 2015 Contributor

Oh, right, derp. I should stop doing code reviews before I get any caffeine in me.

This comment has been minimized.

@crunk1

crunk1 Dec 29, 2015 Author Contributor

All good, thanks for all your help!

@jimbobhickville
Copy link
Contributor

jimbobhickville commented Dec 29, 2015

One last nitpick, but otherwise lgtm.

@erjohnso
Copy link
Member

erjohnso commented Jan 5, 2016

Ok, this lgtm too and I've done some additional testing. The demos/gce_demo.py script is also seeing about a 22% reduction in API calls too. So, 👍, great work @crunk1!

Merging now.

@crunk1
Copy link
Contributor Author

crunk1 commented Jan 6, 2016

This has now been merged: 81a8dbc

@crunk1 crunk1 closed this Jan 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.