RB Close

Review Request #3595 — Created Nov. 30, 2012 and submitted

Information

RBTools
master

Reviewers

RB Close

Allows the user to submit a command 'rb close <rid>' to mark a review request as either 'discarded' or 'submitted'.
On local dev server, tried several combinations of the command and viewed the review request to ensure it match what was desired by the command.

Working cases:
rb close <rid> 
rb close <rid> --description='some desc'
rb close <rid> --type='submitted' 
rb close <rid> --type='submitted' --description='some desc'
rb close <rid> --type='discarded' 
rb close <rid> --type='discarded' --description='some desc'

Bad input cases:
rb close <bad rid>
rb close <rid> --type='bad type' 

Retested with rb-general changes (removal of extra options)
Description From Last Updated

@smacleod : can we rip this out so I don't have to deal with all the coupling related to self.tool …

JS jsintal

Any mentors know how to properly format this line?

JS jsintal

"discarded"

chipx86chipx86

"user can provide"

chipx86chipx86

Shouldn't be needed.

chipx86chipx86

One line, and "number" isn't right. Just say "Returns the review request resource for the given ID."

chipx86chipx86

Where does this come from? I think you mean: sys.exit(1)

chipx86chipx86

type_ is kind of weird. How about close_type?

chipx86chipx86

A nice Pythonism you can use instead is: if close_type not in (SUBMITTED, DISCARDED):

chipx86chipx86

Should be a period after "type"

chipx86chipx86

close_type

chipx86chipx86

No need for parens.

chipx86chipx86

No need for parens.

chipx86chipx86

Slight alignment problem here I think: Either request.update(data={'status': type_, 'description': self.options.description}) Or request.update( data={ 'status': type_, 'description': self.options.description } ) …

SL slchen

Alphabetical.

chipx86chipx86

lowercase the O, I believe.

mike_conleymike_conley

I'm starting to see this pattern repeat itself. Are we sure this isn't something we need in the parent class?

mike_conleymike_conley

Nit: indented too far

mike_conleymike_conley
david
  1. Your diff doesn't seem to have posted correctly.
  2. 
      
JS
JS
JS
JS
  1. 
      
  2. rbtools/commands/rbclose.py (Diff revision 2)
     
     
    Show all issues
    @smacleod : can we rip this out so I don't have to deal with all the coupling related to self.tool (SCM stuff)?
  3. 
      
JS
JS
JS
  1. 
      
  2. rbtools/commands/rbclose.py (Diff revisions 3 - 4)
     
     
    Show all issues
    Any mentors know how to properly format this line?
    1. request = \
          self.root_resource.get_review_requests().get_item(request_id)
      
      Does that give you enough?
    2. Nice, that works, my pep8 compiler doesn't complain. I was also thinking of:
      
      request = self.root_resource.get_review_requests() \
                            .get_item(request_id)
      
      
      But my pep8 compiler complains, saying : 'rbclose.py:96:23: E127 continuation line over-indented for visual indent'
      
      
  3. 
      
JS
JS
JS
JS
SL
  1. 
      
  2. rbtools/commands/rbclose.py (Diff revision 7)
     
     
     
    Show all issues
    Slight alignment problem here I think: 
    
    Either
    
    request.update(data={'status': type_,
                         'description': self.options.description})
    
    Or
    
    request.update(
        data={
                  'status': type_,
                  'description': self.options.description
             }
        )
    
    ?
    1. Sort of the latter, but do:
      
      request.update(data={
          'status': type_,
          'description': self.options.description,
      }))
  3. 
      
chipx86
  1. 
      
  2. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    Show all issues
    "discarded"
  3. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    Show all issues
    "user can provide"
  4. rbtools/commands/rbclose.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Shouldn't be needed.
  5. rbtools/commands/rbclose.py (Diff revision 7)
     
     
     
    Show all issues
    One line, and "number" isn't right. Just say "Returns the review request resource for the given ID."
  6. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    Show all issues
    Where does this come from? I think you mean:
    
    sys.exit(1)
  7. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    Show all issues
    type_ is kind of weird. How about close_type?
  8. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    Show all issues
    A nice Pythonism you can use instead is:
    
    if close_type not in (SUBMITTED, DISCARDED):
  9. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    Show all issues
    Should be a period after "type"
  10. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    Show all issues
    close_type
  11. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    Show all issues
    No need for parens.
  12. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    Show all issues
    No need for parens.
  13. setup.py (Diff revision 7)
     
     
     
    Show all issues
    Alphabetical.
  14. 
      
mike_conley
  1. John: I see your issues raised by Christian have been fixed. Do you have an updated patch?
  2. 
      
JS
mike_conley
  1. 
      
  2. rbtools/commands/rbclose.py (Diff revision 8)
     
     
    Show all issues
    lowercase the O, I believe.
  3. rbtools/commands/rbclose.py (Diff revision 8)
     
     
     
     
     
     
     
    Show all issues
    I'm starting to see this pattern repeat itself. Are we sure this isn't something we need in the parent class?
    1. This is done because of the way options are currently defined. When creating the options on the class, the config has yet to be parsed, so we can't use default values from it.
      
      I'm working on a new way to handle options which fixes this.
      
      You can drop this issue John.
  4. rbtools/commands/rbclose.py (Diff revision 8)
     
     
     
    Show all issues
    Nit: indented too far
  5. setup.py (Diff revision 8)
     
     
    Y'know, when all of these commands land, this is all gonna conflict right here. :)
    1. Yikes! Is there anything I can do to avoid that?
    2. The merges won't be terrible. You'll just have to make the result look correct.
  6. 
      
JS
JS
MW
  1. FWIW, looks reasonable to me, and works on Linux against RB 1.7.1.
  2. 
      
SM
  1. Ship It!
  2. 
      
JS
Review request changed
Status:
Completed
Change Summary:
Pushed to api (cecc50c00b50637b5ded49266640aa7c3e7defcc).