Issue 5453061: Added RetrieveGroups method, and updated RetrieveAllGroups and RetrieveAllMembers in groups client
|
Created:
12 years, 4 months ago by shraddhag Modified:
12 years, 4 months ago CC:
gdata-python-client-library-contributors_googlegroups.com Visibility:
Public. |
Patch Set 1 #
Total comments: 13
Patch Set 2 : Updated according to the comments #
Total comments: 12
Patch Set 3 : Made changes #
Total comments: 4
Patch Set 4 : Made changes #
Total comments: 1
MessagesTotal messages: 13
http://codereview.appspot.com/5453061/diff/1/src/gdata/apps/groups/client.py File src/gdata/apps/groups/client.py (right): http://codereview.appspot.com/5453061/diff/1/src/gdata/apps/groups/client.py#... src/gdata/apps/groups/client.py:125: for a_entry in next_feed.entry: Can be done in a line without a loop I guess link_finder.entry[0:0] = next_feed.entry http://codereview.appspot.com/5453061/diff/1/src/gdata/apps/groups/client.py#... src/gdata/apps/groups/client.py:149: gdata.apps.groups.data.GroupFeed of the groups You are returning atom.data.LinkFinder if I am not wrong. Please return GroupFeed in that case. http://codereview.appspot.com/5453061/diff/1/src/gdata/apps/groups/client.py#... src/gdata/apps/groups/client.py:175: """Retrieve all groups that belong to the given member_id. Use pagination here too. This can be more than 100. http://codereview.appspot.com/5453061/diff/1/src/gdata/apps/groups/client.py#... src/gdata/apps/groups/client.py:262: gdata.apps.groups.data.GroupMemberFeed Check this return too. http://codereview.appspot.com/5453061/diff/1/tests/gdata_tests/apps/groups/li... File tests/gdata_tests/apps/groups/live_client_test.py (right): http://codereview.appspot.com/5453061/diff/1/tests/gdata_tests/apps/groups/li... tests/gdata_tests/apps/groups/live_client_test.py:62: 'https://apps-apis.google.com/a/feeds/groups/'), self.client.auth_scopes) Greater than 80 chars http://codereview.appspot.com/5453061/diff/1/tests/gdata_tests/apps/groups/li... tests/gdata_tests/apps/groups/live_client_test.py:75: , self.client.MakeGroupProvisioningUri(params={'member':'member1'})) Usually comma should stay on first line not in the start of the second line.
Sign in to reply to this message.
http://codereview.appspot.com/5453061/diff/1/src/gdata/apps/groups/client.py File src/gdata/apps/groups/client.py (right): http://codereview.appspot.com/5453061/diff/1/src/gdata/apps/groups/client.py#... src/gdata/apps/groups/client.py:125: for a_entry in next_feed.entry: This seems more intuitive to me. Can we leave it like this On 2011/12/06 18:08:38, gunjansharma wrote: > Can be done in a line without a loop > I guess link_finder.entry[0:0] = next_feed.entry http://codereview.appspot.com/5453061/diff/1/src/gdata/apps/groups/client.py#... src/gdata/apps/groups/client.py:149: gdata.apps.groups.data.GroupFeed of the groups On 2011/12/06 18:08:38, gunjansharma wrote: > You are returning atom.data.LinkFinder if I am not wrong. > Please return GroupFeed in that case. It returns the GroupFeed only as func(str(self.GetFeed(next.href))) function converts the feed into GroupFeed. Updated the return type of RetrieveAllPages method http://codereview.appspot.com/5453061/diff/1/src/gdata/apps/groups/client.py#... src/gdata/apps/groups/client.py:175: """Retrieve all groups that belong to the given member_id. On 2011/12/06 18:08:38, gunjansharma wrote: > Use pagination here too. This can be more than 100. Done. http://codereview.appspot.com/5453061/diff/1/src/gdata/apps/groups/client.py#... src/gdata/apps/groups/client.py:262: gdata.apps.groups.data.GroupMemberFeed On 2011/12/06 18:08:38, gunjansharma wrote: > Check this return too. Done.
Sign in to reply to this message.
http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.py File src/gdata/apps/groups/client.py (right): http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:124: next_feed = func(str(self.GetFeed(next.href))) Check with Claudio. Can we pass the object instead of casting into one? http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:152: ret = self.RetrievePageOfGroups() I think this groups are never getting appended into the final answer. http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:169: #print uri Remove this. I think we should not keep debugging lines in the library code. http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:193: def RetrieveAllGroups(self, member_id, direct_only=False, **kwargs): should it be retrieve_groups? There are two functions by same name. http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:204: ret = self.RetrievePageOfMemberGroups() Same goes here! http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:283: ret = self.RetrievePageOfMembers(group_id=group_id) Same goes here
Sign in to reply to this message.
Also see the comments in the other file you left them in the previous upload. Thanks Gunjan Sharma | Developer Programs Engineer | gunjansharma@google.com | +91 7702534446 On Tue, Dec 6, 2011 at 3:50 PM, <gunjansharma@google.com> wrote: > > http://codereview.appspot.com/**5453061/diff/4001/src/gdata/** > apps/groups/client.py<http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.py> > File src/gdata/apps/groups/client.**py (right): > > http://codereview.appspot.com/**5453061/diff/4001/src/gdata/** > apps/groups/client.py#**newcode124<http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.py#newcode124> > src/gdata/apps/groups/client.**py:124: next_feed = > func(str(self.GetFeed(next.**href))) > Check with Claudio. Can we pass the object instead of casting into one? > > http://codereview.appspot.com/**5453061/diff/4001/src/gdata/** > apps/groups/client.py#**newcode152<http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.py#newcode152> > src/gdata/apps/groups/client.**py:152: ret = self.RetrievePageOfGroups() > I think this groups are never getting appended into the final answer. > > http://codereview.appspot.com/**5453061/diff/4001/src/gdata/** > apps/groups/client.py#**newcode169<http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.py#newcode169> > src/gdata/apps/groups/client.**py:169: #print uri > Remove this. I think we should not keep debugging lines in the library > code. > > http://codereview.appspot.com/**5453061/diff/4001/src/gdata/** > apps/groups/client.py#**newcode193<http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.py#newcode193> > src/gdata/apps/groups/client.**py:193: def RetrieveAllGroups(self, > member_id, direct_only=False, **kwargs): > should it be retrieve_groups? > There are two functions by same name. > > http://codereview.appspot.com/**5453061/diff/4001/src/gdata/** > apps/groups/client.py#**newcode204<http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.py#newcode204> > src/gdata/apps/groups/client.**py:204: ret = > self.**RetrievePageOfMemberGroups() > Same goes here! > > http://codereview.appspot.com/**5453061/diff/4001/src/gdata/** > apps/groups/client.py#**newcode283<http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.py#newcode283> > src/gdata/apps/groups/client.**py:283: ret = > self.RetrievePageOfMembers(**group_id=group_id) > Same goes here > > http://codereview.appspot.com/**5453061/<http://codereview.appspot.com/5453061/> >
Sign in to reply to this message.
http://codereview.appspot.com/5453061/diff/1/src/gdata/apps/groups/client.py File src/gdata/apps/groups/client.py (right): http://codereview.appspot.com/5453061/diff/1/src/gdata/apps/groups/client.py#... src/gdata/apps/groups/client.py:125: for a_entry in next_feed.entry: On 2011/12/06 18:08:38, gunjansharma wrote: > Can be done in a line without a loop > I guess link_finder.entry[0:0] = next_feed.entry Done. http://codereview.appspot.com/5453061/diff/1/tests/gdata_tests/apps/groups/li... File tests/gdata_tests/apps/groups/live_client_test.py (right): http://codereview.appspot.com/5453061/diff/1/tests/gdata_tests/apps/groups/li... tests/gdata_tests/apps/groups/live_client_test.py:62: 'https://apps-apis.google.com/a/feeds/groups/'), self.client.auth_scopes) On 2011/12/06 18:08:38, gunjansharma wrote: > Greater than 80 chars Done. http://codereview.appspot.com/5453061/diff/1/tests/gdata_tests/apps/groups/li... tests/gdata_tests/apps/groups/live_client_test.py:75: , self.client.MakeGroupProvisioningUri(params={'member':'member1'})) On 2011/12/06 18:08:38, gunjansharma wrote: > Usually comma should stay on first line not in the start of the second line. Done. http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.py File src/gdata/apps/groups/client.py (right): http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:124: next_feed = func(str(self.GetFeed(next.href))) On 2011/12/06 20:50:33, gunjansharma wrote: > Check with Claudio. Can we pass the object instead of casting into one? Done. http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:152: ret = self.RetrievePageOfGroups() On 2011/12/06 20:50:33, gunjansharma wrote: > I think this groups are never getting appended into the final answer. Done. http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:169: #print uri On 2011/12/06 20:50:33, gunjansharma wrote: > Remove this. I think we should not keep debugging lines in the library code. Done. http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:193: def RetrieveAllGroups(self, member_id, direct_only=False, **kwargs): On 2011/12/06 20:50:33, gunjansharma wrote: > should it be retrieve_groups? > There are two functions by same name. Done. http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:204: ret = self.RetrievePageOfMemberGroups() On 2011/12/06 20:50:33, gunjansharma wrote: > Same goes here! Done. http://codereview.appspot.com/5453061/diff/4001/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:283: ret = self.RetrievePageOfMembers(group_id=group_id) On 2011/12/06 20:50:33, gunjansharma wrote: > Same goes here Done.
Sign in to reply to this message.
http://codereview.appspot.com/5453061/diff/5002/src/gdata/apps/groups/client.py File src/gdata/apps/groups/client.py (right): http://codereview.appspot.com/5453061/diff/5002/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:111: def RetrieveAllPages(self, feed, desired_class): You can also put default value for desired_class to be GDataFeed http://codereview.appspot.com/5453061/diff/5002/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:122: next = link_finder.GetNextLink() Should it be feed.GetNextLink()? Everywhere in the function.
Sign in to reply to this message.
http://codereview.appspot.com/5453061/diff/5002/src/gdata/apps/groups/client.py File src/gdata/apps/groups/client.py (right): http://codereview.appspot.com/5453061/diff/5002/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:111: def RetrieveAllPages(self, feed, desired_class): On 2011/12/06 22:00:07, gunjansharma wrote: > You can also put default value for desired_class to be GDataFeed Done. http://codereview.appspot.com/5453061/diff/5002/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:122: next = link_finder.GetNextLink() On 2011/12/06 22:00:07, gunjansharma wrote: > Should it be feed.GetNextLink()? > Everywhere in the function. Done.
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
http://codereview.appspot.com/5453061/diff/6004/src/gdata/apps/groups/client.py File src/gdata/apps/groups/client.py (right): http://codereview.appspot.com/5453061/diff/6004/src/gdata/apps/groups/client.... src/gdata/apps/groups/client.py:203: groups_feed = self.RetrievePageOfMemberGroups() Pass the arguments to the function.
Sign in to reply to this message.
|
