Issue 1976046: Adding new analytics feeds

Can't Edit
Can't Publish+Mail
Start Review

Created:
13 years, 8 months ago by api.nickm

Modified:
13 years, 8 months ago

Reviewers:
jcgregorio, jsoneja

CC:
gdata-python-client-library-contributors_googlegroups.com

Visibility:
Public.

Patch Set 2 : updaing code per review #

Total comments: 8

Patch Set 3 : fixing proposed changes from review #

Created: 13 years, 8 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+993 lines, -32 lines)			Patch
A	samples/analytics/mgmt_feed_demo.py	View	1 2	1 chunk	+243 lines, -0 lines	0 comments	Download
M	src/gdata/analytics/client.py	View	1 2	6 chunks	+202 lines, -19 lines	0 comments	Download
M	src/gdata/analytics/data.py	View	1 2	6 chunks	+98 lines, -6 lines	0 comments	Download
M	src/gdata/test_data.py	View		1 chunk	+98 lines, -0 lines	0 comments	Download
M	tests/gdata_tests/analytics/data_test.py	View		3 chunks	+170 lines, -4 lines	0 comments	Download
M	tests/gdata_tests/analytics/live_client_test.py	View	1 2	4 chunks	+16 lines, -3 lines	0 comments	Download
A	tests/gdata_tests/analytics/query_test.py	View		1 chunk	+166 lines, -0 lines	0 comments	Download

Messages

Total messages: 6

Expand All Messages | Collapse All Messages

jsoneja

http://codereview.appspot.com/1976046/diff/1/2 File samples/analytics/mgmt_feed_demo.py (right): http://codereview.appspot.com/1976046/diff/1/2#newcode39 samples/analytics/mgmt_feed_demo.py:39: ACCOUNT_ID = '30481' Do you want to make this ...

13 years, 8 months ago (2010-08-18 22:31:48 UTC) #1

api.nickm

Cool fixed everything except there's an open question on Joe on singular vs plural entities. ...

13 years, 8 months ago (2010-08-18 23:45:33 UTC) #2

Cool fixed everything except there's an open question on Joe on singular vs
plural entities.

http://codereview.appspot.com/1976046/diff/1/2
File samples/analytics/mgmt_feed_demo.py (right):

http://codereview.appspot.com/1976046/diff/1/2#newcode39
samples/analytics/mgmt_feed_demo.py:39: ACCOUNT_ID = '30481'
On 2010/08/18 22:31:49, jsoneja wrote:
> Do you want to make this as a parameter instead of hard-coding a particular ID
?

um for this I think it can be ~all since we only show the account feed by
default. So developers will have to modify the source to see the other feed
outputs.

http://codereview.appspot.com/1976046/diff/1/2#newcode40
samples/analytics/mgmt_feed_demo.py:40: WEB_PROPERTY_ID = 'UA-30481-1'
On 2010/08/18 22:31:49, jsoneja wrote:
> same here.

same comment as above

http://codereview.appspot.com/1976046/diff/1/2#newcode204
samples/analytics/mgmt_feed_demo.py:204: if destination.step:
On 2010/08/18 22:31:49, jsoneja wrote:
> This is more of a comment related to client lib. Would be clearer if the field
> was called 'destination.steps' (i.e., plural) since it's a list.

totally agree. Though the convention for the lib has been to have singular names
represent lists. We do this for entry as well.

Maybe Joe can comment on this?

http://codereview.appspot.com/1976046/diff/1/3
File src/gdata/analytics/client.py (right):

http://codereview.appspot.com/1976046/diff/1/3#newcode84
src/gdata/analytics/client.py:84: Google Analytics and superceeds the Data
Export API Account Feed.
On 2010/08/18 22:31:49, jsoneja wrote:
> 'supercedes'

Done.

http://codereview.appspot.com/1976046/diff/1/3#newcode256
src/gdata/analytics/client.py:256: queryUrl = GoalQuery('123', 'UA-123-1', '555,
On 2010/08/18 22:31:49, jsoneja wrote:
> quote missing after 555

Done.

http://codereview.appspot.com/1976046/diff/1/4
File src/gdata/analytics/data.py (right):

http://codereview.appspot.com/1976046/diff/1/4#newcode18
src/gdata/analytics/data.py:18: Google Analytics Data Export and Management
APIs. Altough both APIs
On 2010/08/18 22:31:49, jsoneja wrote:
> 'Although'

Done.

http://codereview.appspot.com/1976046/diff/1/4#newcode19
src/gdata/analytics/data.py:19: operate on different parts of Google Analtics,
they share common XML
On 2010/08/18 22:31:49, jsoneja wrote:
> 'Analytics'

Done.

jsoneja

I will let Joe comment about singular/plural names. Other than that LGTM. On 2010/08/18 23:45:33, ...

13 years, 8 months ago (2010-08-19 01:34:28 UTC) #3

I will let Joe comment about singular/plural names. Other than that LGTM.


On 2010/08/18 23:45:33, api.nickm wrote:
> Cool fixed everything except there's an open question on Joe on singular vs
> plural entities.
> 
> http://codereview.appspot.com/1976046/diff/1/2
> File samples/analytics/mgmt_feed_demo.py (right):
> 
> http://codereview.appspot.com/1976046/diff/1/2#newcode39
> samples/analytics/mgmt_feed_demo.py:39: ACCOUNT_ID = '30481'
> On 2010/08/18 22:31:49, jsoneja wrote:
> > Do you want to make this as a parameter instead of hard-coding a particular
ID
> ?
> 
> um for this I think it can be ~all since we only show the account feed by
> default. So developers will have to modify the source to see the other feed
> outputs.
> 
> http://codereview.appspot.com/1976046/diff/1/2#newcode40
> samples/analytics/mgmt_feed_demo.py:40: WEB_PROPERTY_ID = 'UA-30481-1'
> On 2010/08/18 22:31:49, jsoneja wrote:
> > same here.
> 
> same comment as above
> 
> http://codereview.appspot.com/1976046/diff/1/2#newcode204
> samples/analytics/mgmt_feed_demo.py:204: if destination.step:
> On 2010/08/18 22:31:49, jsoneja wrote:
> > This is more of a comment related to client lib. Would be clearer if the
field
> > was called 'destination.steps' (i.e., plural) since it's a list.
> 
> totally agree. Though the convention for the lib has been to have singular
names
> represent lists. We do this for entry as well.
> 
> Maybe Joe can comment on this?
> 
> http://codereview.appspot.com/1976046/diff/1/3
> File src/gdata/analytics/client.py (right):
> 
> http://codereview.appspot.com/1976046/diff/1/3#newcode84
> src/gdata/analytics/client.py:84: Google Analytics and superceeds the Data
> Export API Account Feed.
> On 2010/08/18 22:31:49, jsoneja wrote:
> > 'supercedes'
> 
> Done.
> 
> http://codereview.appspot.com/1976046/diff/1/3#newcode256
> src/gdata/analytics/client.py:256: queryUrl = GoalQuery('123', 'UA-123-1',
'555,
> On 2010/08/18 22:31:49, jsoneja wrote:
> > quote missing after 555
> 
> Done.
> 
> http://codereview.appspot.com/1976046/diff/1/4
> File src/gdata/analytics/data.py (right):
> 
> http://codereview.appspot.com/1976046/diff/1/4#newcode18
> src/gdata/analytics/data.py:18: Google Analytics Data Export and Management
> APIs. Altough both APIs
> On 2010/08/18 22:31:49, jsoneja wrote:
> > 'Although'
> 
> Done.
> 
> http://codereview.appspot.com/1976046/diff/1/4#newcode19
> src/gdata/analytics/data.py:19: operate on different parts of Google Analtics,
> they share common XML
> On 2010/08/18 22:31:49, jsoneja wrote:
> > 'Analytics'
> 
> Done.

jcgregorio

About the pluralization, $ find src/gdata/ -name "data.py" | xargs grep "= \[" shows that ...

13 years, 8 months ago (2010-08-19 15:39:10 UTC) #4

api.nickm

thanks. fixed all the comments. Leaving repeated elements as singular. http://codereview.appspot.com/1976046/diff/2002/6001 File samples/analytics/mgmt_feed_demo.py (right): http://codereview.appspot.com/1976046/diff/2002/6001#newcode3 ...

13 years, 8 months ago (2010-08-19 20:43:53 UTC) #5

jcgregorio

13 years, 8 months ago (2010-08-20 17:47:26 UTC) #6

LGTM

Committed:

http://code.google.com/p/gdata-python-client/source/detail?r=e6f80dfaf5b4151c...

   Thanks!  
   -joe

Expand All Messages | Collapse All Messages