Review Bot

Review Request #3032 — Created March 31, 2012 and discarded

Information

ReviewBot

Reviewers

This is an initial prototype of Review Bot. 

Currently all configuration for the Review Board instance, message queue server, and user/password are manual in the code.

 
Description From Last Updated

Typo: Acknowledgements

ME medanat

An empty line or two would be nice here.

ME medanat

self._retried = self._lasturl == args[0]

ME medanat

self._retried = response.code == 401

ME medanat

Is there a reason this is separate initialization call here?

AM ammok

Should the '1.5.2' bit be pulled out into an explanatory constant?

AM ammok

unreachable code? I don't think return True would get executed..

JI jimrrchen

return not cookie.is_expired()

ME medanat

Does this still need to be handled?

AM ammok

Will this method ever reach the end? Does it matter what it returns?

AM ammok

Does this need to be a class variable?

AM ammok

Would it be cleaner to set these values all at once?

AM ammok

Would this be better off being done using HTTPError? http://www.tornadoweb.org/documentation/web.html#everything-else

AM ammok
ME
  1. This all looks very impressive, I didn't get a chance to fully go through the code. I owe you a second review.
    
    Here are some style issues that jumped at me during a quick overview though.
    
    Yazan Medanat
  2. README (Diff revision 1)
     
     
    Show all issues
    Typo: Acknowledgements
  3. reviewbot/processing/__init__.py (Diff revision 1)
     
     
     
    Show all issues
    An empty line or two would be nice here.
  4. reviewbot/processing/api.py (Diff revision 1)
     
     
     
    Show all issues
    self._retried = self._lasturl == args[0]
  5. reviewbot/processing/api.py (Diff revision 1)
     
     
     
    Show all issues
    self._retried = response.code == 401
  6. reviewbot/processing/api.py (Diff revision 1)
     
     
     
     
    Show all issues
    return not cookie.is_expired()
  7. 
      
JI
  1. Great work...too large to review all at once so I am just nitpicking a bit here...
  2. reviewbot/processing/api.py (Diff revision 1)
     
     
    remove the "," since it is a complete sentence
  3. reviewbot/processing/api.py (Diff revision 1)
     
     
    Show all issues
    unreachable code? I don't think return True would get executed..
  4. 
      
AM
  1. I've ignored some of the style things (e.g., one-line summaries) since this is prototype code.
  2. reviewbot/processing/api.py (Diff revision 1)
     
     
    This is more out of curiosity, but what is self.retried used for? I was looking at http://code.reddit.com/docs/urllib2-pysrc.html and it doesn't seem to mention it in the parents (though I suppose it doesn't preclude usage elsewhere).
  3. reviewbot/processing/api.py (Diff revision 1)
     
     
    Show all issues
    Is there a reason this is separate initialization call here?
  4. reviewbot/processing/api.py (Diff revision 1)
     
     
    Show all issues
    Should the '1.5.2' bit be pulled out into an explanatory constant?
  5. reviewbot/processing/api.py (Diff revision 1)
     
     
    Should this return False here or raise an exception? The return value seems to be ignored anyway.
  6. reviewbot/processing/api.py (Diff revision 1)
     
     
    Show all issues
    Does this still need to be handled?
  7. reviewbot/processing/review.py (Diff revision 1)
     
     
    Show all issues
    Will this method ever reach the end? Does it matter what it returns?
  8. reviewbot/processing/review.py (Diff revision 1)
     
     
    Show all issues
    Does this need to be a class variable?
  9. reviewbot/server/handlers.py (Diff revision 1)
     
     
     
     
    Show all issues
    Would it be cleaner to set these values all at once?
  10. reviewbot/server/handlers.py (Diff revision 1)
     
     
    Show all issues
    Would this be better off being done using HTTPError? http://www.tornadoweb.org/documentation/web.html#everything-else
  11. 
      
SM
Review request changed

Status: Discarded

Change Summary:

Code needed serious restructuring and most of the comments were on the API mostly taken from RBTools. 
I'll refer back to this to fix those issues when needed. Hopefully the new API comes soon and I can upgrade.
Loading...