Added a sample to demonstrate updating signature for domain's users

[Ali Afshar, 2012-01-03 13:54:26]

http://codereview.appspot.com/5500098/diff/1/samples/apps/email_settings_sett... File samples/apps/email_settings_setting_signature.py (right): http://codereview.appspot.com/5500098/diff/1/samples/apps/email_settings_sett... samples/apps/email_settings_setting_signature.py:112: thread = SignatureSettingsThread(i, thread_name, There is no need to subclass Thread here, in fact, I believe it is a bad pattern. You could use threading.Thread(target=callable). Also try to bring the code into this module, so that the sample is self-containing. I will re-review after this refactoring.

[gunjansharma, 2012-01-04 06:06:07]

http://codereview.appspot.com/5500098/diff/1/samples/apps/email_settings_sett... File samples/apps/email_settings_setting_signature.py (right): http://codereview.appspot.com/5500098/diff/1/samples/apps/email_settings_sett... samples/apps/email_settings_setting_signature.py:112: thread = SignatureSettingsThread(i, thread_name, On 2012/01/03 13:54:26, Ali Afshar wrote: > There is no need to subclass Thread here, in fact, I believe it is a bad > pattern. You could use threading.Thread(target=callable). Also try to bring the > code into this module, so that the sample is self-containing. > > I will re-review after this refactoring. Done.

[Ali Afshar, 2012-01-04 11:03:31]

http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... File samples/apps/email_settings_setting_signature.py (right): http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:44: class SignatureSettingsException(Exception): Subclass ValueError, or even leave out this exception and raise ValueError below. http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:112: bucket_size = (length + NUM_THREADS -1)/NUM_THREADS Whitespace: NUM_THREADS - 1) / NUM_THREADS http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:113: for i in range(1, NUM_THREADS+1): Whitespace: NUM_THREADS + 1 http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:115: start_index = bucket_size * (i-1) Whitespace: (i - 1) http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:119: threads.append(thread) I don't exactly mind this whole approach, but it might have been much simpler to use the queue class. http://docs.python.org/library/queue.html I don't want to make extra work for you here, but perhaps read that documentation for next time you want to use threads and workers more simply. http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:136: [username, signature] = self._PrepareSignature(profile) use the returned tuple (username, signature) http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:150: phone_number = ','.join(profile.phone_number) Is profile.phone_number an XML Element? If so, this will not work. Maybe you mean ','.join([phone_number.text for phone_number in profile.phone_number]) if it is a list of elements? http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:153: if email.primary and email.primary == 'true': if email.primary == 'true': should be enough if you are expecting email to always have the primary attribute. Otherwise you need to check that first like if hasattr(email, 'primary') and email.primary == 'true', but I think the first is what you want. http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:158: return [username, signature] return a tuple (username, signature) http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:169: [username, subdomain] = email.split('@', 1) Use a tuple (username, subdomain) http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:190: self._CreateSignature(username, signature, tries+1) Whitespace: tries + 1

[gunjansharma, 2012-01-04 11:58:33]

http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... File samples/apps/email_settings_setting_signature.py (right): http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:44: class SignatureSettingsException(Exception): On 2012/01/04 11:03:32, Ali Afshar wrote: > Subclass ValueError, or even leave out this exception and raise ValueError > below. In the previous samples I was asked to do this by Vic. http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:112: bucket_size = (length + NUM_THREADS -1)/NUM_THREADS On 2012/01/04 11:03:32, Ali Afshar wrote: > Whitespace: NUM_THREADS - 1) / NUM_THREADS Done. http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:113: for i in range(1, NUM_THREADS+1): On 2012/01/04 11:03:32, Ali Afshar wrote: > Whitespace: NUM_THREADS + 1 Done. http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:115: start_index = bucket_size * (i-1) On 2012/01/04 11:03:32, Ali Afshar wrote: > Whitespace: (i - 1) Done. http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:119: threads.append(thread) On 2012/01/04 11:03:32, Ali Afshar wrote: > I don't exactly mind this whole approach, but it might have been much simpler to > use the queue class. > > http://docs.python.org/library/queue.html > > I don't want to make extra work for you here, but perhaps read that > documentation for next time you want to use threads and workers more simply. Thanks Ali. Very helpful. Will take care of this from next time :) http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:136: [username, signature] = self._PrepareSignature(profile) On 2012/01/04 11:03:32, Ali Afshar wrote: > use the returned tuple (username, signature) Done. http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:150: phone_number = ','.join(profile.phone_number) On 2012/01/04 11:03:32, Ali Afshar wrote: > Is profile.phone_number an XML Element? If so, this will not work. Maybe you > mean ','.join([phone_number.text for phone_number in profile.phone_number]) if > it is a list of elements? phone_numbers is a list http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:153: if email.primary and email.primary == 'true': On 2012/01/04 11:03:32, Ali Afshar wrote: > if email.primary == 'true': > > should be enough if you are expecting email to always have the primary > attribute. Otherwise you need to check that first like if hasattr(email, > 'primary') and email.primary == 'true', but I think the first is what you want. Not all email will have primary attr. Changed to as you suggested. http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:158: return [username, signature] On 2012/01/04 11:03:32, Ali Afshar wrote: > return a tuple (username, signature) Done. http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:169: [username, subdomain] = email.split('@', 1) On 2012/01/04 11:03:32, Ali Afshar wrote: > Use a tuple (username, subdomain) Done. http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:190: self._CreateSignature(username, signature, tries+1) On 2012/01/04 11:03:32, Ali Afshar wrote: > Whitespace: tries + 1 Done.

[Ali Afshar, 2012-01-04 13:54:08]

http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... File samples/apps/email_settings_setting_signature.py (right): http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:44: class SignatureSettingsException(Exception): On 2012/01/04 11:58:33, gunjansharma wrote: > On 2012/01/04 11:03:32, Ali Afshar wrote: > > Subclass ValueError, or even leave out this exception and raise ValueError > > below. > In the previous samples I was asked to do this by Vic. Fine, in this case, I personally don't think it is necessary.constructor. http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:47: def __init__(self, message): Remove this constructor, it is doing exactly the same thing as the superclass constructor. http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:150: phone_number = ','.join(profile.phone_number) On 2012/01/04 11:58:33, gunjansharma wrote: > On 2012/01/04 11:03:32, Ali Afshar wrote: > > Is profile.phone_number an XML Element? If so, this will not work. Maybe you > > mean ','.join([phone_number.text for phone_number in profile.phone_number]) if > > it is a list of elements? > phone_numbers is a list Looking at src/gdata/contacts/data.py phone_numbers is a list of [gdata.data.PhoneNumber] elements. Please confirm if this is correct or not.

[gunjansharma, 2012-01-04 15:02:09]

http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... File samples/apps/email_settings_setting_signature.py (right): http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:44: class SignatureSettingsException(Exception): On 2012/01/04 13:54:08, Ali Afshar wrote: > On 2012/01/04 11:58:33, gunjansharma wrote: > > On 2012/01/04 11:03:32, Ali Afshar wrote: > > > Subclass ValueError, or even leave out this exception and raise ValueError > > > below. > > In the previous samples I was asked to do this by Vic. > > Fine, in this case, I personally don't think it is necessary.constructor. Done. http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:47: def __init__(self, message): On 2012/01/04 13:54:08, Ali Afshar wrote: > Remove this constructor, it is doing exactly the same thing as the superclass > constructor. Done. http://codereview.appspot.com/5500098/diff/3001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:150: phone_number = ','.join(profile.phone_number) On 2012/01/04 13:54:08, Ali Afshar wrote: > On 2012/01/04 11:58:33, gunjansharma wrote: > > On 2012/01/04 11:03:32, Ali Afshar wrote: > > > Is profile.phone_number an XML Element? If so, this will not work. Maybe you > > > mean ','.join([phone_number.text for phone_number in profile.phone_number]) > if > > > it is a list of elements? > > phone_numbers is a list > > Looking at src/gdata/contacts/data.py phone_numbers is a list of > [gdata.data.PhoneNumber] elements. Please confirm if this is correct or not. My Bad! Changed it.

[Vic Fryzel, 2012-01-04 18:24:47]

http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... File samples/apps/email_settings_setting_signature.py (right): http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:17: """Sample to demonstrate the Email Settings API's signature updation. Please read the Python style guide to determine how to format this block. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:37: # Number of threads to be spawned, other then the main thread Other "than" http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:37: # Number of threads to be spawned, other then the main thread Insert blank line above. "other than", note the 'a' http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:39: # Maximum retries to be done for exponential back-off Maximum number of times to attempt an API request http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:41: SIGNATURE_TEMPLATE = 'Name: %s | Contact: %s | Email: %s' s/Contact/Phone http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:51: consumer_key: [string] The consumerKey of the domain. Again, what does the style guide say? http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:59: def Authorize(self): I don't like how this method isn't called in the constructor, meaning that there is a period of time for which this object will be in an invalid state. As you've copied this mechanism from your other patch, I think that the exception handling requirement that made it necessary this be a separate method, is no longer valid. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:60: """Asks the domain's admin to authorize. authorize what? http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:62: Access to the two APIs needs to be authorized, Reword this sentence. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:140: email = '' How can you combine everything from this line to the break statement into a single line? http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:158: (username, subdomain) = email.split('@', 1) Why the variable name subdomain? http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:164: """Updates the signature for the user. Discuss the exponential back-off that this method does. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:227: raise ValueError('Unknown server error') This should not be a ValueError, and it is consuming the very useful RequestError. Just raise it again.

[Ali Afshar, 2012-01-04 18:34:35]

On 2012/01/04 18:24:47, Vic Fryzel wrote: > http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... > File samples/apps/email_settings_setting_signature.py (right): > > http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... > samples/apps/email_settings_setting_signature.py:17: """Sample to demonstrate > the Email Settings API's signature updation. > Please read the Python style guide to determine how to format this block. > > http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... > samples/apps/email_settings_setting_signature.py:37: # Number of threads to be > spawned, other then the main thread > Other "than" > > http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... > samples/apps/email_settings_setting_signature.py:37: # Number of threads to be > spawned, other then the main thread > Insert blank line above. > > "other than", note the 'a' > > http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... > samples/apps/email_settings_setting_signature.py:39: # Maximum retries to be > done for exponential back-off > Maximum number of times to attempt an API request > > http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... > samples/apps/email_settings_setting_signature.py:41: SIGNATURE_TEMPLATE = 'Name: > %s | Contact: %s | Email: %s' > s/Contact/Phone > > http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... > samples/apps/email_settings_setting_signature.py:51: consumer_key: [string] The > consumerKey of the domain. > Again, what does the style guide say? > > http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... > samples/apps/email_settings_setting_signature.py:59: def Authorize(self): > I don't like how this method isn't called in the constructor, meaning that there > is a period of time for which this object will be in an invalid state. As > you've copied this mechanism from your other patch, I think that the exception > handling requirement that made it necessary this be a separate method, is no > longer valid. > > http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... > samples/apps/email_settings_setting_signature.py:60: """Asks the domain's admin > to authorize. > authorize what? > > http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... > samples/apps/email_settings_setting_signature.py:62: Access to the two APIs > needs to be authorized, > Reword this sentence. > > http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... > samples/apps/email_settings_setting_signature.py:140: email = '' > How can you combine everything from this line to the break statement into a > single line? > > http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... > samples/apps/email_settings_setting_signature.py:158: (username, subdomain) = > email.split('@', 1) > Why the variable name subdomain? > > http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... > samples/apps/email_settings_setting_signature.py:164: """Updates the signature > for the user. > Discuss the exponential back-off that this method does. > > http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... > samples/apps/email_settings_setting_signature.py:227: raise ValueError('Unknown > server error') > This should not be a ValueError, and it is consuming the very useful > RequestError. Just raise it again. This was my fault, I agree with reraising the exception, or just letting it raise.

[gunjansharma, 2012-01-05 06:21:26]

http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... File samples/apps/email_settings_setting_signature.py (right): http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:17: """Sample to demonstrate the Email Settings API's signature updation. On 2012/01/04 18:24:48, Vic Fryzel wrote: > Please read the Python style guide to determine how to format this block. Done. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:37: # Number of threads to be spawned, other then the main thread On 2012/01/04 18:24:48, Vic Fryzel wrote: > Insert blank line above. > > "other than", note the 'a' Done. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:37: # Number of threads to be spawned, other then the main thread On 2012/01/04 18:24:48, Vic Fryzel wrote: > Other "than" Done. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:39: # Maximum retries to be done for exponential back-off On 2012/01/04 18:24:48, Vic Fryzel wrote: > Maximum number of times to attempt an API request Done. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:41: SIGNATURE_TEMPLATE = 'Name: %s | Contact: %s | Email: %s' On 2012/01/04 18:24:48, Vic Fryzel wrote: > s/Contact/Phone Done. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:51: consumer_key: [string] The consumerKey of the domain. On 2012/01/04 18:24:48, Vic Fryzel wrote: > Again, what does the style guide say? Done. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:59: def Authorize(self): On 2012/01/04 18:24:48, Vic Fryzel wrote: > I don't like how this method isn't called in the constructor, meaning that there > is a period of time for which this object will be in an invalid state. As > you've copied this mechanism from your other patch, I think that the exception > handling requirement that made it necessary this be a separate method, is no > longer valid. Done. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:60: """Asks the domain's admin to authorize. On 2012/01/04 18:24:48, Vic Fryzel wrote: > authorize what? Done. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:62: Access to the two APIs needs to be authorized, On 2012/01/04 18:24:48, Vic Fryzel wrote: > Reword this sentence. Done. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:140: email = '' On 2012/01/04 18:24:48, Vic Fryzel wrote: > How can you combine everything from this line to the break statement into a > single line? Done. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:158: (username, subdomain) = email.split('@', 1) On 2012/01/04 18:24:48, Vic Fryzel wrote: > Why the variable name subdomain? Done. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:164: """Updates the signature for the user. On 2012/01/04 18:24:48, Vic Fryzel wrote: > Discuss the exponential back-off that this method does. Done. http://codereview.appspot.com/5500098/diff/7001/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:227: raise ValueError('Unknown server error') On 2012/01/04 18:24:48, Vic Fryzel wrote: > This should not be a ValueError, and it is consuming the very useful > RequestError. Just raise it again. Done.

[gunjansharma, 2012-01-08 08:16:36]

Gentle reminder Thanks Gunjan Sharma | Developer Programs Engineer | gunjansharma@google.com | +91 7702534446 On Thu, Jan 5, 2012 at 11:53 AM, <gunjansharma@google.com> wrote: > http://codereview.appspot.com/**5500098/<http://codereview.appspot.com/5500098/> > 

[Ali Afshar, 2012-01-08 09:03:05]

http://codereview.appspot.com/5500098/diff/9002/samples/apps/email_settings_s... File samples/apps/email_settings_setting_signature.py (right): http://codereview.appspot.com/5500098/diff/9002/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:157: if hasattr(email, 'primary') and email.primary == 'true'][0]) What if the profile has no email addresses, is that possible? http://codereview.appspot.com/5500098/diff/9002/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:239: raise SignatureSettingsException('Invalid Domain') By raise again, I believe Vic meant raise the original exception again, with raise e, or just raise, or just allow the exception to happen.

[gunjansharma, 2012-01-09 11:31:14]

http://codereview.appspot.com/5500098/diff/9002/samples/apps/email_settings_s... File samples/apps/email_settings_setting_signature.py (right): http://codereview.appspot.com/5500098/diff/9002/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:157: if hasattr(email, 'primary') and email.primary == 'true'][0]) On 2012/01/08 09:03:05, Ali Afshar wrote: > What if the profile has no email addresses, is that possible? When creating the user email is a required field. http://codereview.appspot.com/5500098/diff/9002/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:239: raise SignatureSettingsException('Invalid Domain') On 2012/01/08 09:03:05, Ali Afshar wrote: > By raise again, I believe Vic meant raise the original exception again, with > raise e, or just raise, or just allow the exception to happen. @Vic: Should I remove the except block entirely.

[Vic Fryzel, 2012-01-10 01:39:58]

http://codereview.appspot.com/5500098/diff/9002/samples/apps/email_settings_s... File samples/apps/email_settings_setting_signature.py (right): http://codereview.appspot.com/5500098/diff/9002/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:239: raise SignatureSettingsException('Invalid Domain') On 2012/01/09 11:31:14, gunjansharma wrote: > On 2012/01/08 09:03:05, Ali Afshar wrote: > > By raise again, I believe Vic meant raise the original exception again, with > > raise e, or just raise, or just allow the exception to happen. > @Vic: Should I remove the except block entirely. The exceptions seem to make sense in the 403 and 400 case, assuming that the only time a 403 or 400 are thrown are in those cases. The else is not required, though. Do you think we should remove the entire except block?

[gunjansharma, 2012-01-10 06:41:16]

http://codereview.appspot.com/5500098/diff/9002/samples/apps/email_settings_s... File samples/apps/email_settings_setting_signature.py (right): http://codereview.appspot.com/5500098/diff/9002/samples/apps/email_settings_s... samples/apps/email_settings_setting_signature.py:239: raise SignatureSettingsException('Invalid Domain') On 2012/01/10 01:39:58, Vic Fryzel wrote: > On 2012/01/09 11:31:14, gunjansharma wrote: > > On 2012/01/08 09:03:05, Ali Afshar wrote: > > > By raise again, I believe Vic meant raise the original exception again, with > > > raise e, or just raise, or just allow the exception to happen. > > @Vic: Should I remove the except block entirely. > > The exceptions seem to make sense in the 403 and 400 case, assuming that the > only time a 403 or 400 are thrown are in those cases. The else is not required, > though. > > Do you think we should remove the entire except block? Done.

[gunjansharma, 2012-01-13 08:26:00]

Gentle reminder Thanks Gunjan Sharma | Developer Programs Engineer | gunjansharma@google.com | +91 7702534446 On Tue, Jan 10, 2012 at 12:33 PM, <gunjansharma@google.com> wrote: > http://codereview.appspot.com/**5500098/<http://codereview.appspot.com/5500098/> > 

[gunjansharma, 2012-01-16 06:58:32]

On 2012/01/13 08:26:00, gunjansharma wrote: > Gentle reminder > > Thanks > Gunjan Sharma | Developer Programs Engineer | mailto:gunjansharma@google.com | +91 > 7702534446 > > > > On Tue, Jan 10, 2012 at 12:33 PM, <mailto:gunjansharma@google.com> wrote: > > > > http://codereview.appspot.com/**5500098/%3Chttp://codereview.appspot.com/5500...> > > Gentle reminder.

[gunjansharma, 2012-02-16 07:00:51]

Gentle Reminder Thanks Gunjan Sharma | Developer Programs Engineer | gunjansharma@google.com | +91 7702534446 On Mon, Jan 16, 2012 at 12:28 PM, <gunjansharma@google.com> wrote: > On 2012/01/13 08:26:00, gunjansharma wrote: > >> Gentle reminder >> > > Thanks >> Gunjan Sharma | Developer Programs Engineer | >> > mailto:gunjansharma@google.com | +91 > >> 7702534446 >> > > > > On Tue, Jan 10, 2012 at 12:33 PM, <mailto:gunjansharma@google.**com<gunjansharma@google.com> >> > >> > wrote: > > > >> > > http://codereview.appspot.com/****5500098/%3Chttp://** > codereview.appspot.com/**5500098/<http://codereview.appspot.com/**5500098/%3Chttp://codereview.appspot.com/5500098/> > > > >> > >> > > Gentle reminder. > > http://codereview.appspot.com/**5500098/<http://codereview.appspot.com/5500098/> >