Schema info in documentation.

[aiuto, 2011-12-06 20:14:36]

Of course, feel free to reject my design thoughts

http://codereview.appspot.com/5451103/diff/2002/apiclient/discovery.py
File apiclient/discovery.py (right):

http://codereview.appspot.com/5451103/diff/2002/apiclient/discovery.py#newcod...
apiclient/discovery.py:240: schema = SchemaCache(service)
Does this cause a lint error, with schema hiding the module schema.py?

http://codereview.appspot.com/5451103/diff/2002/apiclient/discovery.py#newcod...
apiclient/discovery.py:589:
docs.append(schema.getParsedFromSchema(responseSchema))
I think it might be cleaner here if you divided this into two parts
1. schema.AddSchema("made up name", responseSchema)
2. schema.getParsedFromName("made up name")

But maybe, this is a different facet of my concern in the other file about
SchemaCache doing two separate things. Maybe there should be a cache which does
that, and a pretty printer, which is given a reference to the cache. Then you
ask the printer to getParsedFromSomething().   Maybe like

   SchemaPrinter(schema_cache).getParsedFromName("foo")

http://codereview.appspot.com/5451103/diff/2002/apiclient/schema.py
File apiclient/schema.py (right):

http://codereview.appspot.com/5451103/diff/2002/apiclient/schema.py#newcode68
apiclient/schema.py:68: def __init__(self, discovery):
No Args docs. Is the intent that discovery is a full discovery doc and you are
pulling out the schemas section
to preload the cache?

http://codereview.appspot.com/5451103/diff/2002/apiclient/schema.py#newcode85
apiclient/schema.py:85: seen = copy.copy(seen)
Is this really needed? It seems like seen list never 'pops' - you make a deep
set of calls via _SchemaToStruct and throw seen away.

http://codereview.appspot.com/5451103/diff/2002/apiclient/schema.py#newcode117
apiclient/schema.py:117: return _SchemaToStruct(schema, seen,
dent).to_str(self._getParsedFromName)
I'm having a little trouble following the flow of ownership of the seen list. It
feels like 'seen' should be a member of the SchemaCache class. But, I can see
why it is not, because it is an artifact of printing a particular schema.

Maybe my concern is that this module is called SchemaCache but the primary use
seem to be a Schema doc writer. It's a floor wax and a dessert topping.

http://codereview.appspot.com/5451103/diff/2002/apiclient/schema.py#newcode125
apiclient/schema.py:125: 
Args:

[jcgregorio_google, 2011-12-07 14:43:09]

http://codereview.appspot.com/5451103/diff/2002/apiclient/discovery.py
File apiclient/discovery.py (right):

http://codereview.appspot.com/5451103/diff/2002/apiclient/discovery.py#newcod...
apiclient/discovery.py:240: schema = SchemaCache(service)
Fixed import to be "from apiclient.schema" to avoid the ambiguity.

On 2011/12/06 20:14:36, aiuto wrote:
> Does this cause a lint error, with schema hiding the module schema.py?

http://codereview.appspot.com/5451103/diff/2002/apiclient/discovery.py#newcod...
apiclient/discovery.py:589:
docs.append(schema.getParsedFromSchema(responseSchema))
Should be addressed with the rename.

On 2011/12/06 20:14:36, aiuto wrote:
> I think it might be cleaner here if you divided this into two parts
> 1. schema.AddSchema("made up name", responseSchema)
> 2. schema.getParsedFromName("made up name")
> 
> But maybe, this is a different facet of my concern in the other file about
> SchemaCache doing two separate things. Maybe there should be a cache which
does
> that, and a pretty printer, which is given a reference to the cache. Then you
> ask the printer to getParsedFromSomething().   Maybe like
> 
>    SchemaPrinter(schema_cache).getParsedFromName("foo")
>

http://codereview.appspot.com/5451103/diff/2002/apiclient/schema.py
File apiclient/schema.py (right):

http://codereview.appspot.com/5451103/diff/2002/apiclient/schema.py#newcode68
apiclient/schema.py:68: def __init__(self, discovery):
On 2011/12/06 20:14:36, aiuto wrote:
> No Args docs. Is the intent that discovery is a full discovery doc and you are
> pulling out the schemas section
> to preload the cache?

Done.

http://codereview.appspot.com/5451103/diff/2002/apiclient/schema.py#newcode85
apiclient/schema.py:85: seen = copy.copy(seen)
Ah, doing the pop() is cleaner than doing the copy. Done.

On 2011/12/06 20:14:36, aiuto wrote:
> Is this really needed? It seems like seen list never 'pops' - you make a deep
> set of calls via _SchemaToStruct and throw seen away.

http://codereview.appspot.com/5451103/diff/2002/apiclient/schema.py#newcode117
apiclient/schema.py:117: return _SchemaToStruct(schema, seen,
dent).to_str(self._getParsedFromName)
Fixed seen above. Also renamed the class and methods to make the use clearer.

On 2011/12/06 20:14:36, aiuto wrote:
> I'm having a little trouble following the flow of ownership of the seen list.
It
> feels like 'seen' should be a member of the SchemaCache class. But, I can see
> why it is not, because it is an artifact of printing a particular schema.
> 
> Maybe my concern is that this module is called SchemaCache but the primary use
> seem to be a Schema doc writer. It's a floor wax and a dessert topping.

http://codereview.appspot.com/5451103/diff/2002/apiclient/schema.py#newcode125
apiclient/schema.py:125: 
On 2011/12/06 20:14:36, aiuto wrote:
> Args:

Done.

[aiuto, 2011-12-08 16:51:27]

lgtm

[aiuto, 2011-12-08 16:52:05]

lgtm

[jcgregorio_google, 2011-12-08 17:13:43]

On 2011/12/08 16:52:05, aiuto wrote:
> lgtm

Committed in
http://code.google.com/p/google-api-python-client/source/detail?r=1dea0193773...