|
|
| Created: 13 years, 10 months ago by shraddhag Modified: 13 years, 9 months ago CC: gdata-python-client-library-contributors_googlegroups.com Visibility: Public. | DescriptionReminder: Please review Patch Set 1 # Total comments: 14 Patch Set 2 : Addressed the comments #Patch Set 3 : changed groups names # Total comments: 4 Patch Set 4 : updated #MessagesTotal messages: 9
On 2012/01/06 09:40:52, shraddhag wrote: Reminder: please review. Sign in to reply to this message.
http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups.py File samples/apps/add_users_to_groups.py (right): http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups... samples/apps/add_users_to_groups.py:19: Users retrieved from an organization unit, are added in parallel to different no , between subject and verb http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups... samples/apps/add_users_to_groups.py:68: group: group_id where users are added is group_id a type? You are using it as a type as you do with UserFeed and int http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups... samples/apps/add_users_to_groups.py:85: # Assuming number of groups are less than 20, what happens if there are more than 20 groups? you are not checking it http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups... samples/apps/add_users_to_groups.py:96: parser.add_option('--DOMAIN', I usually expect command-line parameters to be lowercase http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups... samples/apps/add_users_to_groups.py:120: customer_id = customer_id_entry.customer_id you can inline customer_id_entry as this is the only usage Sign in to reply to this message.
http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups.py File samples/apps/add_users_to_groups.py (right): http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups... samples/apps/add_users_to_groups.py:19: Users retrieved from an organization unit, are added in parallel to different On 2012/01/20 23:03:58, Claudio Cherubino wrote: > no , between subject and verb Done. http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups... samples/apps/add_users_to_groups.py:19: Users retrieved from an organization unit, are added in parallel to different On 2012/01/20 23:03:58, Claudio Cherubino wrote: > no , between subject and verb Done. http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups... samples/apps/add_users_to_groups.py:68: group: group_id where users are added On 2012/01/20 23:03:58, Claudio Cherubino wrote: > is group_id a type? You are using it as a type as you do with UserFeed and int Done. http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups... samples/apps/add_users_to_groups.py:68: group: group_id where users are added On 2012/01/20 23:03:58, Claudio Cherubino wrote: > is group_id a type? You are using it as a type as you do with UserFeed and int Done. http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups... samples/apps/add_users_to_groups.py:85: # Assuming number of groups are less than 20, On 2012/01/20 23:03:58, Claudio Cherubino wrote: > what happens if there are more than 20 groups? you are not checking it Done. http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups... samples/apps/add_users_to_groups.py:96: parser.add_option('--DOMAIN', On 2012/01/20 23:03:58, Claudio Cherubino wrote: > I usually expect command-line parameters to be lowercase Done. http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups... samples/apps/add_users_to_groups.py:96: parser.add_option('--DOMAIN', On 2012/01/20 23:03:58, Claudio Cherubino wrote: > I usually expect command-line parameters to be lowercase Done. http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups... samples/apps/add_users_to_groups.py:120: customer_id = customer_id_entry.customer_id On 2012/01/20 23:03:58, Claudio Cherubino wrote: > you can inline customer_id_entry as this is the only usage Done. http://codereview.appspot.com/5519055/diff/1/samples/apps/add_users_to_groups... samples/apps/add_users_to_groups.py:120: customer_id = customer_id_entry.customer_id On 2012/01/20 23:03:58, Claudio Cherubino wrote: > you can inline customer_id_entry as this is the only usage Done. Sign in to reply to this message.
http://codereview.appspot.com/5519055/diff/8001/samples/apps/add_users_to_gro... File samples/apps/add_users_to_groups.py (right): http://codereview.appspot.com/5519055/diff/8001/samples/apps/add_users_to_gro... samples/apps/add_users_to_groups.py:33: Again, 2 lines between imports and first statement of the rest of the file. http://codereview.appspot.com/5519055/diff/8001/samples/apps/add_users_to_gro... samples/apps/add_users_to_groups.py:111: default_groups = ['all-eng', 'programmers', 'misc'] What will happen if these groups do not first exist? Sign in to reply to this message.
http://codereview.appspot.com/5519055/diff/8001/samples/apps/add_users_to_gro... File samples/apps/add_users_to_groups.py (right): http://codereview.appspot.com/5519055/diff/8001/samples/apps/add_users_to_gro... samples/apps/add_users_to_groups.py:33: On 2012/01/22 06:42:59, Vic Fryzel wrote: > Again, 2 lines between imports and first statement of the rest of the file. Done. http://codereview.appspot.com/5519055/diff/8001/samples/apps/add_users_to_gro... samples/apps/add_users_to_groups.py:111: default_groups = ['all-eng', 'programmers', 'misc'] On 2012/01/22 06:42:59, Vic Fryzel wrote: > What will happen if these groups do not first exist? Done. Taking the user input now. If group is not present, an exception is thrown by the thread spawned for that group while the rest of the threads continue running for the other groups. Now I have changed code to create new thread only if the group is present. Sign in to reply to this message. |
