• 
      

    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.