Making a resource subclass and adding datastore model for discovery document in App Engine.

[dhermes, 2012-11-23 22:43:39]

This is an extension of a recent article I wrote:
https://developers.google.com/appengine/articles/efficient_use_of_discovery_b...

I contemplated writing an AppEngineResourceProperty which extends
ndb.PickleProperty but using a PickleProperty stand alone will accomplish all we
need since we use __getstate__ and __setstate__.

(After reading the source), do you think it makes sense to allow
DiscoveryDocument.build to take an expiration which will be passed to the
underlying entity?

[dhermes, 2012-11-26 16:46:17]

I know this review hasn't begun yet, but had an idea I wanted to pass by
you guys.

What do you think about a subclass of httplib2.Http which takes a
Credentials (or subclass) object in it's constructor and is automatically
authorized by these credentials? Since Credentials objects are
serializable, this will allow users to have fully working service objects
immediately after unpickling.


On Fri, Nov 23, 2012 at 2:43 PM, <dhermes@google.com> wrote:

> Reviewers: jcgregorio_google,
>
> Message:
> This is an extension of a recent article I wrote:
> https://developers.google.com/**appengine/articles/efficient_**
>
use_of_discovery_based_apis<https://developers.google.com/appengine/articles/efficient_use_of_discovery_based_apis>
>
> I contemplated writing an AppEngineResourceProperty which extends
> ndb.PickleProperty but using a PickleProperty stand alone will
> accomplish all we need since we use __getstate__ and __setstate__.
>
> (After reading the source), do you think it makes sense to allow
> DiscoveryDocument.build to take an expiration which will be passed to
> the underlying entity?
>
>
>
> Please review this at
https://codereview.appspot.**com/6854090/<https://codereview.appspot.com/6854...
>
> Affected files:
>   A apiclient/appengine.py
>   M apiclient/discovery.py
>   A tests/test_apiclient_**appengine.py
>
>
>


-- 
Danny Hermes
Developer Programs Engineer

[dhermes, 2012-12-06 06:25:29]

Bump.

I'd really like to start this review.


On Thu, Nov 29, 2012 at 1:39 PM, Danny Hermes <dhermes@google.com> wrote:

> Ali and Joe,
>
> Are there any blockers to beginning this review?
>
> I'd really love to get this into a release, it makes using a service
> object on GAE a breeze.
>
>
> On Mon, Nov 26, 2012 at 8:45 AM, Danny Hermes <dhermes@google.com> wrote:
>
>> I know this review hasn't begun yet, but had an idea I wanted to pass by
>> you guys.
>>
>> What do you think about a subclass of httplib2.Http which takes a
>> Credentials (or subclass) object in it's constructor and is automatically
>> authorized by these credentials? Since Credentials objects are
>> serializable, this will allow users to have fully working service objects
>> immediately after unpickling.
>>
>>
>> On Fri, Nov 23, 2012 at 2:43 PM, <dhermes@google.com> wrote:
>>
>>> Reviewers: jcgregorio_google,
>>>
>>> Message:
>>> This is an extension of a recent article I wrote:
>>> https://developers.google.com/**appengine/articles/efficient_**
>>>
use_of_discovery_based_apis<https://developers.google.com/appengine/articles/efficient_use_of_discovery_based_apis>
>>>
>>> I contemplated writing an AppEngineResourceProperty which extends
>>> ndb.PickleProperty but using a PickleProperty stand alone will
>>> accomplish all we need since we use __getstate__ and __setstate__.
>>>
>>> (After reading the source), do you think it makes sense to allow
>>> DiscoveryDocument.build to take an expiration which will be passed to
>>> the underlying entity?
>>>
>>>
>>>
>>> Please review this at
https://codereview.appspot.**com/6854090/<https://codereview.appspot.com/6854...
>>>
>>> Affected files:
>>>   A apiclient/appengine.py
>>>   M apiclient/discovery.py
>>>   A tests/test_apiclient_**appengine.py
>>>
>>>
>>>
>>
>>
>> --
>> Danny Hermes
>> Developer Programs Engineer
>>
>
>
>
> --
> Danny Hermes
> Developer Programs Engineer
>



-- 
Danny Hermes
Developer Programs Engineer

[jcgregorio_google, 2012-12-06 06:28:04]

I am currently traveling, it will be Friday at the earliest that I can
review this.

  -joe

On Wed, Dec 5, 2012 at 1:21 PM, Danny Hermes <dhermes@google.com> wrote:
> Bump.
>
> I'd really like to start this review.
>
>
> On Thu, Nov 29, 2012 at 1:39 PM, Danny Hermes <dhermes@google.com> wrote:
>>
>> Ali and Joe,
>>
>> Are there any blockers to beginning this review?
>>
>> I'd really love to get this into a release, it makes using a service
>> object on GAE a breeze.
>>
>>
>> On Mon, Nov 26, 2012 at 8:45 AM, Danny Hermes <dhermes@google.com> wrote:
>>>
>>> I know this review hasn't begun yet, but had an idea I wanted to pass by
>>> you guys.
>>>
>>> What do you think about a subclass of httplib2.Http which takes a
>>> Credentials (or subclass) object in it's constructor and is automatically
>>> authorized by these credentials? Since Credentials objects are
serializable,
>>> this will allow users to have fully working service objects immediately
>>> after unpickling.
>>>
>>>
>>> On Fri, Nov 23, 2012 at 2:43 PM, <dhermes@google.com> wrote:
>>>>
>>>> Reviewers: jcgregorio_google,
>>>>
>>>> Message:
>>>> This is an extension of a recent article I wrote:
>>>>
>>>>
https://developers.google.com/appengine/articles/efficient_use_of_discovery_b...
>>>>
>>>> I contemplated writing an AppEngineResourceProperty which extends
>>>> ndb.PickleProperty but using a PickleProperty stand alone will
>>>> accomplish all we need since we use __getstate__ and __setstate__.
>>>>
>>>> (After reading the source), do you think it makes sense to allow
>>>> DiscoveryDocument.build to take an expiration which will be passed to
>>>> the underlying entity?
>>>>
>>>>
>>>>
>>>> Please review this at https://codereview.appspot.com/6854090/
>>>>
>>>> Affected files:
>>>>   A apiclient/appengine.py
>>>>   M apiclient/discovery.py
>>>>   A tests/test_apiclient_appengine.py
>>>>
>>>>
>>>
>>>
>>>
>>> --
>>> Danny Hermes
>>> Developer Programs Engineer
>>
>>
>>
>>
>> --
>> Danny Hermes
>> Developer Programs Engineer
>
>
>
>
> --
> Danny Hermes
> Developer Programs Engineer

[dhermes, 2012-12-06 06:57:15]

Thanks


On Wed, Dec 5, 2012 at 11:31 AM, Joe Gregorio <jcgregorio@google.com> wrote:

> I am currently traveling, it will be Friday at the earliest that I can
> review this.
>
>   -joe
>
> On Wed, Dec 5, 2012 at 1:21 PM, Danny Hermes <dhermes@google.com> wrote:
> > Bump.
> >
> > I'd really like to start this review.
> >
> >
> > On Thu, Nov 29, 2012 at 1:39 PM, Danny Hermes <dhermes@google.com>
> wrote:
> >>
> >> Ali and Joe,
> >>
> >> Are there any blockers to beginning this review?
> >>
> >> I'd really love to get this into a release, it makes using a service
> >> object on GAE a breeze.
> >>
> >>
> >> On Mon, Nov 26, 2012 at 8:45 AM, Danny Hermes <dhermes@google.com>
> wrote:
> >>>
> >>> I know this review hasn't begun yet, but had an idea I wanted to pass
> by
> >>> you guys.
> >>>
> >>> What do you think about a subclass of httplib2.Http which takes a
> >>> Credentials (or subclass) object in it's constructor and is
> automatically
> >>> authorized by these credentials? Since Credentials objects are
> serializable,
> >>> this will allow users to have fully working service objects immediately
> >>> after unpickling.
> >>>
> >>>
> >>> On Fri, Nov 23, 2012 at 2:43 PM, <dhermes@google.com> wrote:
> >>>>
> >>>> Reviewers: jcgregorio_google,
> >>>>
> >>>> Message:
> >>>> This is an extension of a recent article I wrote:
> >>>>
> >>>>
>
https://developers.google.com/appengine/articles/efficient_use_of_discovery_b...
> >>>>
> >>>> I contemplated writing an AppEngineResourceProperty which extends
> >>>> ndb.PickleProperty but using a PickleProperty stand alone will
> >>>> accomplish all we need since we use __getstate__ and __setstate__.
> >>>>
> >>>> (After reading the source), do you think it makes sense to allow
> >>>> DiscoveryDocument.build to take an expiration which will be passed to
> >>>> the underlying entity?
> >>>>
> >>>>
> >>>>
> >>>> Please review this at https://codereview.appspot.com/6854090/
> >>>>
> >>>> Affected files:
> >>>>   A apiclient/appengine.py
> >>>>   M apiclient/discovery.py
> >>>>   A tests/test_apiclient_appengine.py
> >>>>
> >>>>
> >>>
> >>>
> >>>
> >>> --
> >>> Danny Hermes
> >>> Developer Programs Engineer
> >>
> >>
> >>
> >>
> >> --
> >> Danny Hermes
> >> Developer Programs Engineer
> >
> >
> >
> >
> > --
> > Danny Hermes
> > Developer Programs Engineer
>



-- 
Danny Hermes
Developer Programs Engineer

[jcgregorio_google, 2012-12-07 20:37:04]

https://codereview.appspot.com/6854090/diff/1/apiclient/appengine.py
File apiclient/appengine.py (right):

https://codereview.appspot.com/6854090/diff/1/apiclient/appengine.py#newcode24
apiclient/appengine.py:24: def _TotalSeconds(delta):
I'm not a big fan of this approach, the classes know too much about discovery,
Resource objects, etc.

I'd much rather see the addition of a new method:

  apiclient.discovery.build_from_document_cache(cache)

where the cache object does the fetching/caching of the discovery doc. Then the
create an NDB backed cache for discovery documents that can be passed to
build_from_document_cache().

[dhermes, 2012-12-07 21:54:39]

https://codereview.appspot.com/6854090/diff/1/apiclient/appengine.py
File apiclient/appengine.py (right):

https://codereview.appspot.com/6854090/diff/1/apiclient/appengine.py#newcode24
apiclient/appengine.py:24: def _TotalSeconds(delta):
Before I do a re-write can we chat about design a bit?

Also, does this comment refer to the line it's on or just the file in general?

On 2012/12/07 20:37:04, jcgregorio_google wrote:
> I'm not a big fan of this approach, the classes know too much about discovery,
> Resource objects, etc.
> 
> I'd much rather see the addition of a new method:
> 
>   apiclient.discovery.build_from_document_cache(cache)
> 
> where the cache object does the fetching/caching of the discovery doc. Then
the
> create an NDB backed cache for discovery documents that can be passed to
> build_from_document_cache().

[jcgregorio_google, 2012-12-07 21:56:14]

On Fri, Dec 7, 2012 at 4:54 PM,  <dhermes@google.com> wrote:
>
> https://codereview.appspot.com/6854090/diff/1/apiclient/appengine.py
> File apiclient/appengine.py (right):
>
>
https://codereview.appspot.com/6854090/diff/1/apiclient/appengine.py#newcode24
> apiclient/appengine.py:24: def _TotalSeconds(delta):
> Before I do a re-write can we chat about design a bit?

Sure, let's set up a VC for next week.

>
> Also, does this comment refer to the line it's on or just the file in
> general?

Just the file in general.

 -joe

>
>
> On 2012/12/07 20:37:04, jcgregorio_google wrote:
>>
>> I'm not a big fan of this approach, the classes know too much about
>
> discovery,
>>
>> Resource objects, etc.
>
>
>> I'd much rather see the addition of a new method:
>
>
>>    apiclient.discovery.build_from_document_cache(cache)
>
>
>> where the cache object does the fetching/caching of the discovery doc.
>
> Then the
>>
>> create an NDB backed cache for discovery documents that can be passed
>
> to
>>
>> build_from_document_cache().
>
>
> https://codereview.appspot.com/6854090/

[dhermes, 2012-12-07 22:03:12]

VC invite sent


On Fri, Dec 7, 2012 at 1:55 PM, Joe Gregorio <jcgregorio@google.com> wrote:

> On Fri, Dec 7, 2012 at 4:54 PM,  <dhermes@google.com> wrote:
> >
> > https://codereview.appspot.com/6854090/diff/1/apiclient/appengine.py
> > File apiclient/appengine.py (right):
> >
> >
>
https://codereview.appspot.com/6854090/diff/1/apiclient/appengine.py#newcode24
> > apiclient/appengine.py:24: def _TotalSeconds(delta):
> > Before I do a re-write can we chat about design a bit?
>
> Sure, let's set up a VC for next week.
>
> >
> > Also, does this comment refer to the line it's on or just the file in
> > general?
>
> Just the file in general.
>
>  -joe
>
> >
> >
> > On 2012/12/07 20:37:04, jcgregorio_google wrote:
> >>
> >> I'm not a big fan of this approach, the classes know too much about
> >
> > discovery,
> >>
> >> Resource objects, etc.
> >
> >
> >> I'd much rather see the addition of a new method:
> >
> >
> >>    apiclient.discovery.build_from_document_cache(cache)
> >
> >
> >> where the cache object does the fetching/caching of the discovery doc.
> >
> > Then the
> >>
> >> create an NDB backed cache for discovery documents that can be passed
> >
> > to
> >>
> >> build_from_document_cache().
> >
> >
> > https://codereview.appspot.com/6854090/
>



-- 
Danny Hermes
Developer Programs Engineer

[dhermes, 2013-02-12 06:16:28]

I started to update this patch after the review started, but for some reason the
docs files updated in the interim also started uploading.

I think I'll start a fresh review tomorrow, if you don't mind, since this one is
a bit borked now.

[jcgregorio_google, 2013-02-12 13:44:17]

SGTM

On Tue, Feb 12, 2013 at 1:16 AM,  <dhermes@google.com> wrote:
> I started to update this patch after the review started, but for some
> reason the docs files updated in the interim also started uploading.
>
> I think I'll start a fresh review tomorrow, if you don't mind, since
> this one is a bit borked now.
>
> https://codereview.appspot.com/6854090/

[dhermes, 2013-02-13 21:56:25]

Closing this version.

Re-opened as https://codereview.appspot.com/7306107/