Issue 5528104: Added a sample to demonstarte Email Audit api's email monitor CRUD operations
|
Created:
14 years, 5 months ago by gunjansharma Modified:
14 years, 5 months ago CC:
gdata-python-client-library-contributors_googlegroups.com Visibility:
Public. |
Patch Set 1 #
Total comments: 17
Patch Set 2 : Patch1 #Patch Set 3 : patch1 #MessagesTotal messages: 9
http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... File samples/apps/email_audit_email_monitoring.py (right): http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:35: You shouldn't need to define __init__ for this class. I believe the following works: class EmailMonitoringException(Exception): """Exception class for EmailMonitoring, shows appropriate error message.""" I don't think you even need: pass() due to the docstring. http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:52: consumer_key: [string] The consumerKey of the domain. The types of arguments are generally omitted from pydoc args. http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:59: self._Authorize() Having simple constructors is better. I would move the call to _Authorize out into it's own step (perhaps making it public). http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:108: """Takes a valid date as input. It might be helpful to describe what is a valid date in the docstring. http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:242: opts, args = getopt.getopt(sys.argv[1:], '', ['consumer_key=', This is completely optional, but the argparse module makes reading args (and constructing the usage string) much easier: http://docs.python.org/library/argparse.html#module-argparse.
Sign in to reply to this message.
http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... File samples/apps/email_audit_email_monitoring.py (right): http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:35: On 2012/01/20 23:47:16, danielholevoet wrote: > You shouldn't need to define __init__ for this class. I believe the following > works: > > class EmailMonitoringException(Exception): > """Exception class for EmailMonitoring, shows appropriate error message.""" > > I don't think you even need: > > pass() > > due to the docstring. Actually this in consistence with previous samples where I have built a class similar to this. http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:52: consumer_key: [string] The consumerKey of the domain. On 2012/01/20 23:47:16, danielholevoet wrote: > The types of arguments are generally omitted from pydoc args. Done. http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:59: self._Authorize() On 2012/01/20 23:47:16, danielholevoet wrote: > Having simple constructors is better. I would move the call to _Authorize out > into it's own step (perhaps making it public). This has been a question of debate in the previous samples as well. Finally Vic asked me to include it in the constructor itself. http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:108: """Takes a valid date as input. On 2012/01/20 23:47:16, danielholevoet wrote: > It might be helpful to describe what is a valid date in the docstring. Done. http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:242: opts, args = getopt.getopt(sys.argv[1:], '', ['consumer_key=', On 2012/01/20 23:47:16, danielholevoet wrote: > This is completely optional, but the argparse module makes reading args (and > constructing the usage string) much easier: > http://docs.python.org/library/argparse.html#module-argparse. I will take this into account from next time. Almost all the gdata apps samples uses getopt module. Thanks for the suggestion :)
Sign in to reply to this message.
http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... File samples/apps/email_audit_email_monitoring.py (right): http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:35: On 2012/01/21 07:41:53, gunjansharma wrote: > On 2012/01/20 23:47:16, danielholevoet wrote: > > You shouldn't need to define __init__ for this class. I believe the following > > works: > > > > class EmailMonitoringException(Exception): > > """Exception class for EmailMonitoring, shows appropriate error message.""" > > > > I don't think you even need: > > > > pass() > > > > due to the docstring. > Actually this in consistence with previous samples where I have built a class > similar to this. Are you sure previous samples didn't actually do something in the constructor? Here, what Dan is saying is that this constructor doesn't do anything, and therefore doesn't need to be here. http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:59: self._Authorize() On 2012/01/21 07:41:53, gunjansharma wrote: > On 2012/01/20 23:47:16, danielholevoet wrote: > > Having simple constructors is better. I would move the call to _Authorize out > > into it's own step (perhaps making it public). > This has been a question of debate in the previous samples as well. Finally Vic > asked me to include it in the constructor itself. +1. @Dan I asked him to put it here so that the class could not be initialized into a bad state, as other methods depend on authorization having succeeded.
Sign in to reply to this message.
http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... File samples/apps/email_audit_email_monitoring.py (right): http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:35: On 2012/01/22 06:32:21, Vic Fryzel wrote: > On 2012/01/21 07:41:53, gunjansharma wrote: > > On 2012/01/20 23:47:16, danielholevoet wrote: > > > You shouldn't need to define __init__ for this class. I believe the > following > > > works: > > > > > > class EmailMonitoringException(Exception): > > > """Exception class for EmailMonitoring, shows appropriate error > message.""" > > > > > > I don't think you even need: > > > > > > pass() > > > > > > due to the docstring. > > Actually this in consistence with previous samples where I have built a class > > similar to this. > > Are you sure previous samples didn't actually do something in the constructor? > Here, what Dan is saying is that this constructor doesn't do anything, and > therefore doesn't need to be here. Actually yes. Although I can remove this if you want. http://code.google.com/p/gdata-python-client/source/browse/samples/apps/email...
Sign in to reply to this message.
http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... File samples/apps/email_audit_email_monitoring.py (right): http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:35: On 2012/01/23 12:16:28, gunjansharma wrote: > On 2012/01/22 06:32:21, Vic Fryzel wrote: > > On 2012/01/21 07:41:53, gunjansharma wrote: > > > On 2012/01/20 23:47:16, danielholevoet wrote: > > > > You shouldn't need to define __init__ for this class. I believe the > > following > > > > works: > > > > > > > > class EmailMonitoringException(Exception): > > > > """Exception class for EmailMonitoring, shows appropriate error > > message.""" > > > > > > > > I don't think you even need: > > > > > > > > pass() > > > > > > > > due to the docstring. > > > Actually this in consistence with previous samples where I have built a > class > > > similar to this. > > > > Are you sure previous samples didn't actually do something in the constructor? > > > Here, what Dan is saying is that this constructor doesn't do anything, and > > therefore doesn't need to be here. > Actually yes. Although I can remove this if you want. > http://code.google.com/p/gdata-python-client/source/browse/samples/apps/email... Since the other samples do it this way, you can leave it. In future sample sets where this practice hasn't been established, I'd leave the constructors out. http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:59: self._Authorize() On 2012/01/22 06:32:21, Vic Fryzel wrote: > On 2012/01/21 07:41:53, gunjansharma wrote: > > On 2012/01/20 23:47:16, danielholevoet wrote: > > > Having simple constructors is better. I would move the call to _Authorize > out > > > into it's own step (perhaps making it public). > > This has been a question of debate in the previous samples as well. Finally > Vic > > asked me to include it in the constructor itself. > > +1. @Dan I asked him to put it here so that the class could not be initialized > into a bad state, as other methods depend on authorization having succeeded. Since other samples do it this way, leave it for consistency. In future samples, using a factory method might be a better approach. E.g: @staticmethod def CreateAuthorizedInstance(): o = EmailMonitoring() o.Authorize() return o You still need to stub out Authorize when testing this method, but not when just testing the basic constructor (which will be very useful when writing tests).
Sign in to reply to this message.
LGTM with constructor removed. http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... File samples/apps/email_audit_email_monitoring.py (right): http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:35: Gunjan can you just delete the constructor really quick? That might be easiest. http://codereview.appspot.com/5528104/diff/1/samples/apps/email_audit_email_m... samples/apps/email_audit_email_monitoring.py:59: self._Authorize() On 2012/01/23 18:24:46, danielholevoet wrote: > On 2012/01/22 06:32:21, Vic Fryzel wrote: > > On 2012/01/21 07:41:53, gunjansharma wrote: > > > On 2012/01/20 23:47:16, danielholevoet wrote: > > > > Having simple constructors is better. I would move the call to _Authorize > > out > > > > into it's own step (perhaps making it public). > > > This has been a question of debate in the previous samples as well. Finally > > Vic > > > asked me to include it in the constructor itself. > > > > +1. @Dan I asked him to put it here so that the class could not be > initialized > > into a bad state, as other methods depend on authorization having succeeded. > > Since other samples do it this way, leave it for consistency. > > In future samples, using a factory method might be a better approach. E.g: > > @staticmethod > def CreateAuthorizedInstance(): > o = EmailMonitoring() > o.Authorize() > return o > > You still need to stub out Authorize when testing this method, but not when just > testing the basic constructor (which will be very useful when writing tests). +1. Gunjan please do this in the future, but it's not necessary now.
Sign in to reply to this message.
|
