new Maps-Engine sample

[jcgregorio_google, 2013-12-02 20:09:07]

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/README
File samples/maps_engine/README (right):

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/README#new...
samples/maps_engine/README:6: api: maps-engine
Should be mapsengine to match the id of the API.

[jlivni, 2013-12-02 20:13:11]

mapsengine api keyword fixed

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/README
File samples/maps_engine/README (right):

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/README#new...
samples/maps_engine/README:6: api: maps-engine
On 2013/12/02 20:09:07, jcgregorio_google wrote:
> Should be mapsengine to match the id of the API.

Done.

[jcgregorio_google, 2013-12-02 20:13:41]

On 2013/12/02 20:13:11, jlivni wrote:
> mapsengine api keyword fixed
> 
> https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/README
> File samples/maps_engine/README (right):
> 
>
https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/README#new...
> samples/maps_engine/README:6: api: maps-engine
> On 2013/12/02 20:09:07, jcgregorio_google wrote:
> > Should be mapsengine to match the id of the API.
> 
> Done.

LGTM

[Ali Afshar, 2013-12-02 20:17:56]

LGTM, minor nits

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
File samples/maps_engine/maps_engine.py (right):

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:31: If you do not enter a shapefile, it will
upload the included "polygons"
Add trailing period.

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:45: import argparse
Alphabetic order for imports (sorry).

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:53: logging.basicConfig(level=logging.INFO)
Add blank line after this.

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:64: pass
pass is redundant here if you have a docstring. Omit it please.

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:71: http: http, authenticated http object.
Is this authorized, rather than authenticated?

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:122: # TODO(jlivni): Make these ACLs a flag
instead of hardcoding here.
Add this or remove the TODO.

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:128: create_url = BASE_URL + "upload"
Yous interpolation or ''.join for concatenation like you do for filename above.

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:147: logging.debug("upload url is %s", url)
Why debug here?

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:172: if status in ["processed", "complete",
"ready"]:
Extract this to a constant set e.g. SUCCESFUL_STATUSES = set(["..", ..])

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:184: def CheckResponse(response, url):
Rename: Fail/Raise if bad response

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:193: 
Remove blank line.

[jlivni, 2013-12-02 21:53:00]

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
File samples/maps_engine/maps_engine.py (right):

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:31: If you do not enter a shapefile, it will
upload the included "polygons"
On 2013/12/02 20:17:57, Ali Afshar wrote:
> Add trailing period.

Done.

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:45: import argparse
On 2013/12/02 20:17:57, Ali Afshar wrote:
> Alphabetic order for imports (sorry).

To clarify, you want sample_tools inline with default python modules?

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:53: logging.basicConfig(level=logging.INFO)
On 2013/12/02 20:17:57, Ali Afshar wrote:
> Add blank line after this.

Done.

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:64: pass
On 2013/12/02 20:17:57, Ali Afshar wrote:
> pass is redundant here if you have a docstring. Omit it please.

Done.

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:71: http: http, authenticated http object.
On 2013/12/02 20:17:57, Ali Afshar wrote:
> Is this authorized, rather than authenticated?

Done.

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:122: # TODO(jlivni): Make these ACLs a flag
instead of hardcoding here.
On 2013/12/02 20:17:57, Ali Afshar wrote:
> Add this or remove the TODO.

Done.

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:128: create_url = BASE_URL + "upload"
On 2013/12/02 20:17:57, Ali Afshar wrote:
> Yous interpolation or ''.join for concatenation like you do for filename
above.

Done.

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:147: logging.debug("upload url is %s", url)
On 2013/12/02 20:17:57, Ali Afshar wrote:
> Why debug here?

Common developer feedback on this API is confusion on urls, since there's a
different base url for uploading the data vs posting the metadata.

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:172: if status in ["processed", "complete",
"ready"]:
On 2013/12/02 20:17:57, Ali Afshar wrote:
> Extract this to a constant set e.g. SUCCESFUL_STATUSES = set(["..", ..])

Done.

https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
samples/maps_engine/maps_engine.py:193: 
On 2013/12/02 20:17:57, Ali Afshar wrote:
> Remove blank line.

Done.

[Ali Afshar, 2013-12-03 00:58:36]

On 2013/12/02 21:53:00, jlivni wrote:
>
https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
> File samples/maps_engine/maps_engine.py (right):
> 
>
https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
> samples/maps_engine/maps_engine.py:31: If you do not enter a shapefile, it
will
> upload the included "polygons"
> On 2013/12/02 20:17:57, Ali Afshar wrote:
> > Add trailing period.
> 
> Done.
> 
>
https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
> samples/maps_engine/maps_engine.py:45: import argparse
> On 2013/12/02 20:17:57, Ali Afshar wrote:
> > Alphabetic order for imports (sorry).
> 
> To clarify, you want sample_tools inline with default python modules?

Yes, that can come after. Usually grouping stdlib modules together is best.

>
https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
> samples/maps_engine/maps_engine.py:53: logging.basicConfig(level=logging.INFO)
> On 2013/12/02 20:17:57, Ali Afshar wrote:
> > Add blank line after this.
> 
> Done.
> 
>
https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
> samples/maps_engine/maps_engine.py:64: pass
> On 2013/12/02 20:17:57, Ali Afshar wrote:
> > pass is redundant here if you have a docstring. Omit it please.
> 
> Done.
> 
>
https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
> samples/maps_engine/maps_engine.py:71: http: http, authenticated http object.
> On 2013/12/02 20:17:57, Ali Afshar wrote:
> > Is this authorized, rather than authenticated?
> 
> Done.
> 
>
https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
> samples/maps_engine/maps_engine.py:122: # TODO(jlivni): Make these ACLs a flag
> instead of hardcoding here.
> On 2013/12/02 20:17:57, Ali Afshar wrote:
> > Add this or remove the TODO.
> 
> Done.
> 
>
https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
> samples/maps_engine/maps_engine.py:128: create_url = BASE_URL + "upload"
> On 2013/12/02 20:17:57, Ali Afshar wrote:
> > Yous interpolation or ''.join for concatenation like you do for filename
> above.
> 
> Done.
> 
>
https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
> samples/maps_engine/maps_engine.py:147: logging.debug("upload url is %s", url)
> On 2013/12/02 20:17:57, Ali Afshar wrote:
> > Why debug here?
> 
> Common developer feedback on this API is confusion on urls, since there's a
> different base url for uploading the data vs posting the metadata.
> 
>
https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
> samples/maps_engine/maps_engine.py:172: if status in ["processed", "complete",
> "ready"]:
> On 2013/12/02 20:17:57, Ali Afshar wrote:
> > Extract this to a constant set e.g. SUCCESFUL_STATUSES = set(["..", ..])
> 
> Done.
> 
>
https://codereview.appspot.com/36120043/diff/1/samples/maps_engine/maps_engin...
> samples/maps_engine/maps_engine.py:193: 
> On 2013/12/02 20:17:57, Ali Afshar wrote:
> > Remove blank line.
> 
> Done.

[jlivni, 2013-12-03 18:36:42]

Thanks guys,

Anything else I need to do to get this into the codebase?


On Mon, Dec 2, 2013 at 4:58 PM, <afshar@google.com> wrote:

> https://codereview.appspot.com/36120043/
>

[Ali Afshar, 2013-12-03 23:24:04]

LGTM


On Tue, Dec 3, 2013 at 10:36 AM, Josh Livni <jlivni@google.com> wrote:

> Thanks guys,
>
> Anything else I need to do to get this into the codebase?
>
>
> On Mon, Dec 2, 2013 at 4:58 PM, <afshar@google.com> wrote:
>
>> https://codereview.appspot.com/36120043/
>>
>
>


-- 
Ali Afshar | google.com/+alihafshar | Google Developer Relations

[jcgregorio_google, 2013-12-05 13:08:40]

On 2013/12/03 23:24:04, Ali Afshar wrote:
> LGTM
> 
> 
> On Tue, Dec 3, 2013 at 10:36 AM, Josh Livni <mailto:jlivni@google.com> wrote:
> 
> > Thanks guys,
> >
> > Anything else I need to do to get this into the codebase?
> >
> >
> > On Mon, Dec 2, 2013 at 4:58 PM, <mailto:afshar@google.com> wrote:
> >
> >> https://codereview.appspot.com/36120043/
> >>
> >
> >
> 
> 
> -- 
> Ali Afshar | google.com/+alihafshar | Google Developer Relations

Now committed.