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)
     
     
    @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)
     
     
    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)
     
     
     
    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)
     
     
    "discarded"
  3. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    "user can provide"
  4. rbtools/commands/rbclose.py (Diff revision 7)
     
     
     
     
     
     
     
     
     
     
     
     
    Shouldn't be needed.
  5. rbtools/commands/rbclose.py (Diff revision 7)
     
     
     
    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)
     
     
    Where does this come from? I think you mean:
    
    sys.exit(1)
  7. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    type_ is kind of weird. How about close_type?
  8. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    A nice Pythonism you can use instead is:
    
    if close_type not in (SUBMITTED, DISCARDED):
  9. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    Should be a period after "type"
  10. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    close_type
  11. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    No need for parens.
  12. rbtools/commands/rbclose.py (Diff revision 7)
     
     
    No need for parens.
  13. setup.py (Diff revision 7)
     
     
     
    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)
     
     
    lowercase the O, I believe.
  3. rbtools/commands/rbclose.py (Diff revision 8)
     
     
     
     
     
     
     
    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)
     
     
     
    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: Closed (submitted)

Change Summary:

Pushed to api (cecc50c00b50637b5ded49266640aa7c3e7defcc).
Loading...