Replaced multiple get_review_request methods with an equivalent utility function

Review Request #6348 — Created Sept. 20, 2014 and submitted

asalahli
RBTools
master
8f14536...
rbtools, students

There was a method called get_review_request() in 4 different classes with identical
bodies. They have now been replaced with a utility function in rbtools.utils.commands.

Manually tested attach, close, post, publish commands.

It seems difficult to add new unit tests to verify the correctness of this new
change. And the existing unit tests pass even if the commands are fatally broken.

Description From Last Updated

'APIError' imported but unused

reviewbotreviewbot

'Option' imported but unused

reviewbotreviewbot

Blank line between this and the if statement.

chipx86chipx86

Can you add a blank line after this?

chipx86chipx86

There still is another version of this function in rbtools.hooks.common, but they differ in the exception they are raising. One ...

AS asalahli

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot

Col: 1 W293 blank line contains whitespace

reviewbotreviewbot
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/attach.py
        rbtools/utils/review.py
        rbtools/commands/publish.py
        rbtools/commands/close.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/attach.py
        rbtools/utils/review.py
        rbtools/commands/publish.py
        rbtools/commands/close.py
    
    
  2. rbtools/commands/close.py (Diff revision 1)
     
     
     'APIError' imported but unused
    
  3. rbtools/commands/publish.py (Diff revision 1)
     
     
     'Option' imported but unused
    
  4. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/attach.py
        rbtools/utils/review.py
        rbtools/commands/publish.py
        rbtools/commands/close.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/attach.py
        rbtools/utils/review.py
        rbtools/commands/publish.py
        rbtools/commands/close.py
    
    
  2. 
      
AS
  1. 
      
  2. rbtools/utils/review.py (Diff revision 2)
     
     

    There still is another version of this function in rbtools.hooks.common, but they differ in the exception they are raising. One raises CommandError and other does HookError. Should I leave them that way?

    1. Might be nice to have a ReviewRequestNotFound error instead, and then the other callers can catch and raise their own respective error based on the text contents of that one.

    2. Actually, then we're really doing nothing better than each function having their own get_review_request... Let me think on this.

    3. But then every caller will do:

      try:
          review_request = get_review_request(request_id, api_root):
      except ReviewRequestNotFound, e:
          raise SpecificError("Error getting review request %s: %s"
                              % (review_request_id, e))
      

      which is basically same with the get_review_request function in the first place

    4. However, if we make ReviewRequestNotFound inherit from both CommandError and HookError, then catching either of those exceptions will also catch ReviewRequestNotFound

    5. That's true, but it doesn't really make sense to say that a "review request not found" is a type of a "hook" or "command" failure. It's really its own thing.

      I was having some talks with Steven about the organization of rbtools/utils/, which I'm afraid is getting a bit multi-purpose. For now, however, let's go ahead and make a rbtools/utils/commands.py and put it there. It can be a place for utility commands. We'll just leave the hooks usage alone.

      I think we'll want to do other work for cleaning up utils, but that's probably a larger task requiring more thought than we should give right now.

  3. 
      
chipx86
  1. 
      
  2. rbtools/commands/post.py (Diff revision 2)
     
     

    Blank line between this and the if statement.

  3. rbtools/commands/publish.py (Diff revision 2)
     
     

    Can you add a blank line after this?

  4. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/attach.py
        rbtools/utils/review.py
        rbtools/commands/publish.py
        rbtools/commands/close.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/attach.py
        rbtools/utils/review.py
        rbtools/commands/publish.py
        rbtools/commands/close.py
    
    
  2. rbtools/commands/post.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  3. rbtools/commands/publish.py (Diff revision 3)
     
     
    Col: 1
     W293 blank line contains whitespace
    
  4. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/attach.py
        rbtools/utils/review.py
        rbtools/commands/publish.py
        rbtools/commands/close.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/attach.py
        rbtools/utils/review.py
        rbtools/commands/publish.py
        rbtools/commands/close.py
    
    
  2. 
      
AS
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/attach.py
        rbtools/utils/commands.py
        rbtools/commands/publish.py
        rbtools/commands/close.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        rbtools/commands/post.py
        rbtools/commands/attach.py
        rbtools/utils/commands.py
        rbtools/commands/publish.py
        rbtools/commands/close.py
    
    
  2. 
      
chipx86
  1. Ship It!

  2. 
      
AS
AS
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-0.6.x (845188b)
Loading...