Issue 5452063: Added Groups provsioning start guide sample and updated multidomain one
|
Created:
12 years, 4 months ago by gunjansharma Modified:
12 years, 4 months ago CC:
gdata-python-client-library-contributors_googlegroups.com Visibility:
Public. |
Patch Set 1 #
Total comments: 29
Patch Set 2 : Resolved 1st set of comments #Patch Set 3 : Resolved comments 2 #
Total comments: 10
Patch Set 4 : Resolved comments set3 #Patch Set 5 : set 4 #Patch Set 6 : set5 #
Total comments: 6
Patch Set 7 : set5 #
MessagesTotal messages: 23
http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... File samples/apps/groups_provisioning_quick_start_example.py (right): http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:56: scope=SCOPE, user_agent=USER_AGENT) Don't put this code in the object constructor, expose another method to perform authorization. This will also allow other developers to replace the OAuth 2.0 command-line flow with the flow of their choice. http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:100: print '' This this line? For extra line to be printed you can you '/n' in previous print. http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:111: print '' Same here http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:138: break description is optional field in create method. http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:152: print choice email permission is optional field in create method. Do we need to force getting a value for it http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:170: group_entry.SetGroupName(group_data.group_name) I feel this goes against the conventional way of accessing the fields group_entry.group_name. May be this way we are setting wrong example before people. @Claudio: Can you tell if this is correct? http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:179: """Retrieves a single group.""" Doesn't lint requires to leave a line after 1 line comment
Sign in to reply to this message.
http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... File samples/apps/groups_provisioning_quick_start_example.py (right): http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:27: class GroupData(object): can't you use the GroupEntry class defined in the library? http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:148: print choice will this print the number the user just typed? If so, why? http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:170: group_entry.SetGroupName(group_data.group_name) On 2011/12/07 16:08:43, shraddhag wrote: > I feel this goes against the conventional way of accessing the fields > group_entry.group_name. May be this way we are setting wrong example before > people. > @Claudio: Can you tell if this is correct? This code is valid, when settings group_name we actually call SetGroupName, but this is some kind of implementation detail and Python programmers are more used to setting the property directly (unlike Java programmers who are used to getters/setters). So, replace this with: group_entry.group_name = group_data.group_name http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:185: """Retrieves all the groups that belongs to a user.""" s/Retrieves all the groups that belongs to a user./Retrieves all the groups the user is a member of.
Sign in to reply to this message.
http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... File samples/apps/groups_provisioning_quick_start_example.py (right): http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:27: class GroupData(object): On 2011/12/07 16:23:02, Claudio Cherubino wrote: > can't you use the GroupEntry class defined in the library? Actually it will have many other fields and functions. I am using this just as a kind of enum plus I need them to be initialized to empty strings. http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:56: scope=SCOPE, user_agent=USER_AGENT) On 2011/12/07 16:08:43, shraddhag wrote: > Don't put this code in the object constructor, expose another method to perform > authorization. This will also allow other developers to replace the OAuth 2.0 > command-line flow with the flow of their choice. Done. http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:100: print '' On 2011/12/07 16:08:43, shraddhag wrote: > This this line? For extra line to be printed you can you '/n' in previous print. This seemed more appropriate. Will check with Claudio http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:111: print '' On 2011/12/07 16:08:43, shraddhag wrote: > Same here Done. http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:138: break On 2011/12/07 16:08:43, shraddhag wrote: > description is optional field in create method. I don't think so. Atleast for the service architecture it isn't. But you can give an empty string accommodated that now. http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:148: print choice On 2011/12/07 16:23:02, Claudio Cherubino wrote: > will this print the number the user just typed? If so, why? Done. http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:152: print choice On 2011/12/07 16:08:43, shraddhag wrote: > email permission is optional field in create method. Do we need to force getting > a value for it Same here http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:170: group_entry.SetGroupName(group_data.group_name) On 2011/12/07 16:23:02, Claudio Cherubino wrote: > On 2011/12/07 16:08:43, shraddhag wrote: > > I feel this goes against the conventional way of accessing the fields > > group_entry.group_name. May be this way we are setting wrong example before > > people. > > @Claudio: Can you tell if this is correct? > > This code is valid, when settings group_name we actually call SetGroupName, but > this is some kind of implementation detail and Python programmers are more used > to setting the property directly (unlike Java programmers who are used to > getters/setters). So, replace this with: > > group_entry.group_name = group_data.group_name Done. http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:179: """Retrieves a single group.""" On 2011/12/07 16:08:43, shraddhag wrote: > Doesn't lint requires to leave a line after 1 line comment No. http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:185: """Retrieves all the groups that belongs to a user.""" On 2011/12/07 16:23:02, Claudio Cherubino wrote: > s/Retrieves all the groups that belongs to a user./Retrieves all the groups the > user is a member of. Done.
Sign in to reply to this message.
Gentle Reminder. Thanks Gunjan Sharma | Developer Programs Engineer | gunjansharma@google.com | +91 7702534446 On Thu, Dec 8, 2011 at 4:07 AM, <gunjansharma@google.com> wrote: > > http://codereview.appspot.com/**5452063/diff/1/samples/apps/** > groups_provisioning_quick_**start_example.py<http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning_quick_start_example.py> > File samples/apps/groups_**provisioning_quick_start_**example.py (right): > > http://codereview.appspot.com/**5452063/diff/1/samples/apps/** > groups_provisioning_quick_**start_example.py#newcode27<http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning_quick_start_example.py#newcode27> > samples/apps/groups_**provisioning_quick_start_**example.py:27: class > GroupData(object): > On 2011/12/07 16:23:02, Claudio Cherubino wrote: > >> can't you use the GroupEntry class defined in the library? >> > Actually it will have many other fields and functions. I am using this > just as a kind of enum plus I need them to be initialized to empty > strings. > > > http://codereview.appspot.com/**5452063/diff/1/samples/apps/** > groups_provisioning_quick_**start_example.py#newcode56<http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning_quick_start_example.py#newcode56> > samples/apps/groups_**provisioning_quick_start_**example.py:56: > scope=SCOPE, > user_agent=USER_AGENT) > On 2011/12/07 16:08:43, shraddhag wrote: > >> Don't put this code in the object constructor, expose another method >> > to perform > >> authorization. This will also allow other developers to replace the >> > OAuth 2.0 > >> command-line flow with the flow of their choice. >> > > Done. > > > http://codereview.appspot.com/**5452063/diff/1/samples/apps/** > groups_provisioning_quick_**start_example.py#newcode100<http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning_quick_start_example.py#newcode100> > samples/apps/groups_**provisioning_quick_start_**example.py:100: print '' > On 2011/12/07 16:08:43, shraddhag wrote: > >> This this line? For extra line to be printed you can you '/n' in >> > previous print. > This seemed more appropriate. Will check with Claudio > > > http://codereview.appspot.com/**5452063/diff/1/samples/apps/** > groups_provisioning_quick_**start_example.py#newcode111<http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning_quick_start_example.py#newcode111> > samples/apps/groups_**provisioning_quick_start_**example.py:111: print '' > On 2011/12/07 16:08:43, shraddhag wrote: > >> Same here >> > > Done. > > > http://codereview.appspot.com/**5452063/diff/1/samples/apps/** > groups_provisioning_quick_**start_example.py#newcode138<http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning_quick_start_example.py#newcode138> > samples/apps/groups_**provisioning_quick_start_**example.py:138: break > On 2011/12/07 16:08:43, shraddhag wrote: > >> description is optional field in create method. >> > I don't think so. Atleast for the service architecture it isn't. > But you can give an empty string accommodated that now. > > > http://codereview.appspot.com/**5452063/diff/1/samples/apps/** > groups_provisioning_quick_**start_example.py#newcode148<http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning_quick_start_example.py#newcode148> > samples/apps/groups_**provisioning_quick_start_**example.py:148: print > choice > On 2011/12/07 16:23:02, Claudio Cherubino wrote: > >> will this print the number the user just typed? If so, why? >> > > Done. > > > http://codereview.appspot.com/**5452063/diff/1/samples/apps/** > groups_provisioning_quick_**start_example.py#newcode152<http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning_quick_start_example.py#newcode152> > samples/apps/groups_**provisioning_quick_start_**example.py:152: print > choice > On 2011/12/07 16:08:43, shraddhag wrote: > >> email permission is optional field in create method. Do we need to >> > force getting > >> a value for it >> > Same here > > > http://codereview.appspot.com/**5452063/diff/1/samples/apps/** > groups_provisioning_quick_**start_example.py#newcode170<http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning_quick_start_example.py#newcode170> > samples/apps/groups_**provisioning_quick_start_**example.py:170: > group_entry.SetGroupName(**group_data.group_name) > On 2011/12/07 16:23:02, Claudio Cherubino wrote: > >> On 2011/12/07 16:08:43, shraddhag wrote: >> > I feel this goes against the conventional way of accessing the >> > fields > >> > group_entry.group_name. May be this way we are setting wrong example >> > before > >> > people. >> > @Claudio: Can you tell if this is correct? >> > > This code is valid, when settings group_name we actually call >> > SetGroupName, but > >> this is some kind of implementation detail and Python programmers are >> > more used > >> to setting the property directly (unlike Java programmers who are used >> > to > >> getters/setters). So, replace this with: >> > > group_entry.group_name = group_data.group_name >> > > Done. > > > http://codereview.appspot.com/**5452063/diff/1/samples/apps/** > groups_provisioning_quick_**start_example.py#newcode179<http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning_quick_start_example.py#newcode179> > samples/apps/groups_**provisioning_quick_start_**example.py:179: > """Retrieves a single group.""" > On 2011/12/07 16:08:43, shraddhag wrote: > >> Doesn't lint requires to leave a line after 1 line comment >> > No. > > > http://codereview.appspot.com/**5452063/diff/1/samples/apps/** > groups_provisioning_quick_**start_example.py#newcode185<http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning_quick_start_example.py#newcode185> > samples/apps/groups_**provisioning_quick_start_**example.py:185: > """Retrieves all the groups that belongs to a user.""" > On 2011/12/07 16:23:02, Claudio Cherubino wrote: > >> s/Retrieves all the groups that belongs to a user./Retrieves all the >> > groups the > >> user is a member of. >> > > Done. > > http://codereview.appspot.com/**5452063/<http://codereview.appspot.com/5452063/> >
Sign in to reply to this message.
http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... File samples/apps/groups_provisioning_quick_start_example.py (right): http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:27: class GroupData(object): On 2011/12/07 22:37:05, gunjansharma wrote: > On 2011/12/07 16:23:02, Claudio Cherubino wrote: > > can't you use the GroupEntry class defined in the library? > Actually it will have many other fields and functions. I am using this just as a > kind of enum plus I need them to be initialized to empty strings. The GroupEntry class doesn't have much more: http://code.google.com/p/gdata-python-client/source/browse/src/gdata/apps/gro... http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:56: scope=SCOPE, user_agent=USER_AGENT) On 2011/12/07 22:37:05, gunjansharma wrote: > On 2011/12/07 16:08:43, shraddhag wrote: > > Don't put this code in the object constructor, expose another method to > perform > > authorization. This will also allow other developers to replace the OAuth 2.0 > > command-line flow with the flow of their choice. > > Done. What you have done doesn't change much, you moved the code into a separate function but that is still called by the constructor. Call it from the main() instead http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:100: print '' On 2011/12/07 22:37:05, gunjansharma wrote: > On 2011/12/07 16:08:43, shraddhag wrote: > > This this line? For extra line to be printed you can you '/n' in previous > print. > This seemed more appropriate. Will check with Claudio Both ways are fine, your call. http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:138: break On 2011/12/07 22:37:05, gunjansharma wrote: > On 2011/12/07 16:08:43, shraddhag wrote: > > description is optional field in create method. > I don't think so. Atleast for the service architecture it isn't. > But you can give an empty string accommodated that now. The API doesn't require description and email permissions. Also, you are using the GroupsProvisioningClient class which (correctly) doesn't require them.
Sign in to reply to this message.
http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... File samples/apps/groups_provisioning_quick_start_example.py (right): http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:27: class GroupData(object): On 2011/12/12 11:45:14, Claudio Cherubino wrote: > On 2011/12/07 22:37:05, gunjansharma wrote: > > On 2011/12/07 16:23:02, Claudio Cherubino wrote: > > > can't you use the GroupEntry class defined in the library? > > Actually it will have many other fields and functions. I am using this just as > a > > kind of enum plus I need them to be initialized to empty strings. > > The GroupEntry class doesn't have much more: > > http://code.google.com/p/gdata-python-client/source/browse/src/gdata/apps/gro... Done. http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:56: scope=SCOPE, user_agent=USER_AGENT) On 2011/12/12 11:45:14, Claudio Cherubino wrote: > On 2011/12/07 22:37:05, gunjansharma wrote: > > On 2011/12/07 16:08:43, shraddhag wrote: > > > Don't put this code in the object constructor, expose another method to > > perform > > > authorization. This will also allow other developers to replace the OAuth > 2.0 > > > command-line flow with the flow of their choice. > > > > Done. > > What you have done doesn't change much, you moved the code into a separate > function but that is still called by the constructor. Call it from the main() > instead Done. http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:100: print '' On 2011/12/12 11:45:14, Claudio Cherubino wrote: > On 2011/12/07 22:37:05, gunjansharma wrote: > > On 2011/12/07 16:08:43, shraddhag wrote: > > > This this line? For extra line to be printed you can you '/n' in previous > > print. > > This seemed more appropriate. Will check with Claudio > > Both ways are fine, your call. Done. http://codereview.appspot.com/5452063/diff/1/samples/apps/groups_provisioning... samples/apps/groups_provisioning_quick_start_example.py:138: break On 2011/12/12 11:45:14, Claudio Cherubino wrote: > On 2011/12/07 22:37:05, gunjansharma wrote: > > On 2011/12/07 16:08:43, shraddhag wrote: > > > description is optional field in create method. > > I don't think so. Atleast for the service architecture it isn't. > > But you can give an empty string accommodated that now. > > The API doesn't require description and email permissions. > Also, you are using the GroupsProvisioningClient class which (correctly) doesn't > require them. Accommodated empty strings to leave the fields blank.
Sign in to reply to this message.
http://codereview.appspot.com/5452063/diff/12002/samples/apps/groups_provisio... File samples/apps/groups_provisioning_quick_start_example.py (right): http://codereview.appspot.com/5452063/diff/12002/samples/apps/groups_provisio... samples/apps/groups_provisioning_quick_start_example.py:122: group_data.SetGroupId(self._GetValidGroupId()) why did you change back all calls to use setters and getters instead of accessing the properties directly? http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... File samples/apps/multidomain_quick_start_example.py (right): http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... samples/apps/multidomain_quick_start_example.py:71: self.multidomain_client = gdata.apps.multidomain.client.MultiDomainProvisioningClient( isn't this line too long?
Sign in to reply to this message.
http://codereview.appspot.com/5452063/diff/12002/samples/apps/groups_provisio... File samples/apps/groups_provisioning_quick_start_example.py (right): http://codereview.appspot.com/5452063/diff/12002/samples/apps/groups_provisio... samples/apps/groups_provisioning_quick_start_example.py:122: group_data.SetGroupId(self._GetValidGroupId()) On 2011/12/12 14:24:18, Claudio Cherubino wrote: > why did you change back all calls to use setters and getters instead of > accessing the properties directly? Done. http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... File samples/apps/multidomain_quick_start_example.py (right): http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... samples/apps/multidomain_quick_start_example.py:71: self.multidomain_client = gdata.apps.multidomain.client.MultiDomainProvisioningClient( On 2011/12/12 14:24:18, Claudio Cherubino wrote: > isn't this line too long? Can't change
Sign in to reply to this message.
http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... File samples/apps/multidomain_quick_start_example.py (right): http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... samples/apps/multidomain_quick_start_example.py:71: self.multidomain_client = gdata.apps.multidomain.client.MultiDomainProvisioningClient( On 2011/12/13 09:21:30, gunjansharma wrote: > On 2011/12/12 14:24:18, Claudio Cherubino wrote: > > isn't this line too long? > Can't change What about this? self.multidomain_client = gdata.apps.multidomain.client.MultiDomainProvisioningClient( moving the part after the = to the next line with 4 spaces. http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... samples/apps/multidomain_quick_start_example.py:149: % (extra_stmt)) Are the extra parenthesis really needed? Can't you just write this? sys.stdout.write('Enter password for the user%s: ' % extra_stmt)
Sign in to reply to this message.
http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... File samples/apps/multidomain_quick_start_example.py (right): http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... samples/apps/multidomain_quick_start_example.py:71: self.multidomain_client = gdata.apps.multidomain.client.MultiDomainProvisioningClient( On 2011/12/13 09:29:09, Claudio Cherubino wrote: > On 2011/12/13 09:21:30, gunjansharma wrote: > > On 2011/12/12 14:24:18, Claudio Cherubino wrote: > > > isn't this line too long? > > Can't change > > What about this? > > self.multidomain_client = > gdata.apps.multidomain.client.MultiDomainProvisioningClient( > > moving the part after the = to the next line with 4 spaces. Error: File "multidomain_quick_start_example.py", line 71 self.multidomain_client = ^ SyntaxError: invalid syntax http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... samples/apps/multidomain_quick_start_example.py:149: % (extra_stmt)) On 2011/12/13 09:29:09, Claudio Cherubino wrote: > Are the extra parenthesis really needed? > Can't you just write this? > > sys.stdout.write('Enter password for the user%s: ' > % extra_stmt) > Done.
Sign in to reply to this message.
http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... File samples/apps/multidomain_quick_start_example.py (right): http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... samples/apps/multidomain_quick_start_example.py:71: self.multidomain_client = gdata.apps.multidomain.client.MultiDomainProvisioningClient( On 2011/12/13 09:44:10, gunjansharma wrote: > On 2011/12/13 09:29:09, Claudio Cherubino wrote: > > On 2011/12/13 09:21:30, gunjansharma wrote: > > > On 2011/12/12 14:24:18, Claudio Cherubino wrote: > > > > isn't this line too long? > > > Can't change > > > > What about this? > > > > self.multidomain_client = > > gdata.apps.multidomain.client.MultiDomainProvisioningClient( > > > > moving the part after the = to the next line with 4 spaces. > > Error: > File "multidomain_quick_start_example.py", line 71 > self.multidomain_client = > ^ > SyntaxError: invalid syntax Sorry I forgot the parenthesis for implicit line joining (http://www.corp.google.com/eng/doc/pyguide.xml#Line_length): self.multidomain_client = ( gdata.apps.multidomain.client.MultiDomainProvisioningClient(...))
Sign in to reply to this message.
http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... File samples/apps/multidomain_quick_start_example.py (right): http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... samples/apps/multidomain_quick_start_example.py:71: self.multidomain_client = gdata.apps.multidomain.client.MultiDomainProvisioningClient( On 2011/12/13 10:02:26, Claudio Cherubino wrote: > On 2011/12/13 09:44:10, gunjansharma wrote: > > On 2011/12/13 09:29:09, Claudio Cherubino wrote: > > > On 2011/12/13 09:21:30, gunjansharma wrote: > > > > On 2011/12/12 14:24:18, Claudio Cherubino wrote: > > > > > isn't this line too long? > > > > Can't change > > > > > > What about this? > > > > > > self.multidomain_client = > > > gdata.apps.multidomain.client.MultiDomainProvisioningClient( > > > > > > moving the part after the = to the next line with 4 spaces. > > > > Error: > > File "multidomain_quick_start_example.py", line 71 > > self.multidomain_client = > > ^ > > SyntaxError: invalid syntax > > Sorry I forgot the parenthesis for implicit line joining > (http://www.corp.google.com/eng/doc/pyguide.xml#Line_length%29: > > self.multidomain_client = ( > gdata.apps.multidomain.client.MultiDomainProvisioningClient(...)) Ohh din't knew that. Thanks Done.
Sign in to reply to this message.
On 2011/12/13 10:10:15, gunjansharma wrote: > http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... > File samples/apps/multidomain_quick_start_example.py (right): > > http://codereview.appspot.com/5452063/diff/12002/samples/apps/multidomain_qui... > samples/apps/multidomain_quick_start_example.py:71: self.multidomain_client = > gdata.apps.multidomain.client.MultiDomainProvisioningClient( > On 2011/12/13 10:02:26, Claudio Cherubino wrote: > > On 2011/12/13 09:44:10, gunjansharma wrote: > > > On 2011/12/13 09:29:09, Claudio Cherubino wrote: > > > > On 2011/12/13 09:21:30, gunjansharma wrote: > > > > > On 2011/12/12 14:24:18, Claudio Cherubino wrote: > > > > > > isn't this line too long? > > > > > Can't change > > > > > > > > What about this? > > > > > > > > self.multidomain_client = > > > > gdata.apps.multidomain.client.MultiDomainProvisioningClient( > > > > > > > > moving the part after the = to the next line with 4 spaces. > > > > > > Error: > > > File "multidomain_quick_start_example.py", line 71 > > > self.multidomain_client = > > > ^ > > > SyntaxError: invalid syntax > > > > Sorry I forgot the parenthesis for implicit line joining > > (http://www.corp.google.com/eng/doc/pyguide.xml#Line_length%2529: > > > > self.multidomain_client = ( > > gdata.apps.multidomain.client.MultiDomainProvisioningClient(...)) > Ohh din't knew that. Thanks Done. LGTM, if Shraddha has no more comments, then you can submit it
Sign in to reply to this message.
http://codereview.appspot.com/5452063/diff/12005/samples/apps/groups_provisio... File samples/apps/groups_provisioning_quick_start_example.py (right): http://codereview.appspot.com/5452063/diff/12005/samples/apps/groups_provisio... samples/apps/groups_provisioning_quick_start_example.py:47: """Creates a groups provisioning client using OAuth2.0 flow.""" You may chose to leave blank line after doc comment or not leave at all places to make it consistent. http://codereview.appspot.com/5452063/diff/12005/samples/apps/groups_provisio... samples/apps/groups_provisioning_quick_start_example.py:100: member_entry: [MemberEntry] contains all the data about the group member. GroupMemberEntry http://codereview.appspot.com/5452063/diff/12005/samples/apps/groups_provisio... samples/apps/groups_provisioning_quick_start_example.py:147: """Creates a new group.""" Consider blank line. Please make consistent everywhere
Sign in to reply to this message.
http://codereview.appspot.com/5452063/diff/12005/samples/apps/groups_provisio... File samples/apps/groups_provisioning_quick_start_example.py (right): http://codereview.appspot.com/5452063/diff/12005/samples/apps/groups_provisio... samples/apps/groups_provisioning_quick_start_example.py:47: """Creates a groups provisioning client using OAuth2.0 flow.""" On 2011/12/13 11:37:47, shraddhag wrote: > You may chose to leave blank line after doc comment or not leave at all places > to make it consistent. Consistent with what? I have changed everything to have no blank line after doc comment. http://codereview.appspot.com/5452063/diff/12005/samples/apps/groups_provisio... samples/apps/groups_provisioning_quick_start_example.py:100: member_entry: [MemberEntry] contains all the data about the group member. On 2011/12/13 11:37:47, shraddhag wrote: > GroupMemberEntry Done. http://codereview.appspot.com/5452063/diff/12005/samples/apps/groups_provisio... samples/apps/groups_provisioning_quick_start_example.py:147: """Creates a new group.""" On 2011/12/13 11:37:47, shraddhag wrote: > Consider blank line. Please make consistent everywhere Done.
Sign in to reply to this message.
LGTM Shraddha Gupta Developer Programs Engineer Hyderabad, Google India. On Tue, Dec 13, 2011 at 5:17 PM, <gunjansharma@google.com> wrote: > > http://codereview.appspot.com/**5452063/diff/12005/samples/** > apps/groups_provisioning_**quick_start_example.py<http://codereview.appspot.com/5452063/diff/12005/samples/apps/groups_provisioning_quick_start_example.py> > File samples/apps/groups_**provisioning_quick_start_**example.py (right): > > http://codereview.appspot.com/**5452063/diff/12005/samples/** > apps/groups_provisioning_**quick_start_example.py#**newcode47<http://codereview.appspot.com/5452063/diff/12005/samples/apps/groups_provisioning_quick_start_example.py#newcode47> > samples/apps/groups_**provisioning_quick_start_**example.py:47: > """Creates a > groups provisioning client using OAuth2.0 flow.""" > On 2011/12/13 11:37:47, shraddhag wrote: > >> You may chose to leave blank line after doc comment or not leave at >> > all places > >> to make it consistent. >> > Consistent with what? I have changed everything to have no blank line > after doc comment. > > > http://codereview.appspot.com/**5452063/diff/12005/samples/** > apps/groups_provisioning_**quick_start_example.py#**newcode100<http://codereview.appspot.com/5452063/diff/12005/samples/apps/groups_provisioning_quick_start_example.py#newcode100> > samples/apps/groups_**provisioning_quick_start_**example.py:100: > member_entry: [MemberEntry] contains all the data about the group > member. > On 2011/12/13 11:37:47, shraddhag wrote: > >> GroupMemberEntry >> > > Done. > > > http://codereview.appspot.com/**5452063/diff/12005/samples/** > apps/groups_provisioning_**quick_start_example.py#**newcode147<http://codereview.appspot.com/5452063/diff/12005/samples/apps/groups_provisioning_quick_start_example.py#newcode147> > samples/apps/groups_**provisioning_quick_start_**example.py:147: > """Creates > a new group.""" > On 2011/12/13 11:37:47, shraddhag wrote: > >> Consider blank line. Please make consistent everywhere >> > > Done. > > http://codereview.appspot.com/**5452063/<http://codereview.appspot.com/5452063/> >
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
