RB Attach

Review Request #3599 — Created Dec. 1, 2012 and submitted

Information

RBTools
master

Reviewers

A command that allows the user to attach a file to a specific review request.

Args: rb attach <request_id> <path/to/file>
Options: --description (caption), --filename (custom filename)
Tried the following on dev server, all of which were successful:

rb attach <request_id> <path/to/file>
rb attach <request_id> <path/to/file> --description='some desc'
rb attach <request_id> <path/to/file> --filename='custom filename'
rb attach <request_id> <path/to/file> --description='some desc' --filename='custom filename'

rb attach <invalid_request_id>
rb patch <invalid_request_id> <invalid/path/to/file>
rb attach <invalid/path/to/file>

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

Make sure these are alphabetical.

chipx86chipx86

No blank line here.

chipx86chipx86

For this and every other option, one parameter per line.

chipx86chipx86

Think I'd prefer this just be --caption

chipx86chipx86

Think I'd rather we just fix that function then. We shouldn't pollute this file. Instead of doing options.foo, you can …

chipx86chipx86

Curious, maybe this is just something I'm not aware of with the new command infrastructure, but why aren't these just …

chipx86chipx86

Always treat these like proper sentences. It's missing a period.

chipx86chipx86

Trim the spaces.

chipx86chipx86

I agree. Let's do that.

chipx86chipx86

Pretty sure this fits on one line.

chipx86chipx86

Indentation is off.

chipx86chipx86

Things like this should be removed.

chipx86chipx86

The code's pretty self-explanatory. No need for the comment.

chipx86chipx86

Please use %s. This helps us localize.

chipx86chipx86

Maybe say what the default is.

chipx86chipx86

I can't imagine this looking right. Please properly terminate a string when you need to wrap and start it up …

chipx86chipx86

Alignment is off.

chipx86chipx86

These should be alphabetical

SL slchen

You can probably condense: file_attachments = request.get_file_attachments() ... file_attachments.upload_attachment(filename, content, self.options.caption) To just: request.get_file_attachments().upload_attachment(filename, content, self.options.caption)

SL slchen

Should the the error be logged?

SL slchen

Two blank lines.

chipx86chipx86

Always should be proper sentences. Terminate with a period.

chipx86chipx86

Are these needed?

chipx86chipx86

Same comments as your diff change. Can you go through those and apply those same changes to this function? That …

chipx86chipx86

Do we not want to use die()?

chipx86chipx86

Surely there may be errors. We need to check the status and report them.

chipx86chipx86

Alphabetical.

chipx86chipx86

I think I'd prefer: request = self.root_resource \ .get_review_requests() \ .get_item(request_id)

mike_conleymike_conley

I think you can do this with a Python "with" structure since RB 1.7 is moving to Python 2.5+; The …

SL slchen

I *think* this: request.get_file_attachments() \ .upload_attachment(filename, content, self.options.caption) Does PEP8 like that?

mike_conleymike_conley

Indented too much

mike_conleymike_conley
chipx86
  1. Is this rb attach, or rb patch? The summary and other fields disagree.
  2. 
      
chipx86
  1. 
      
  2. rbtools/commands/rbattach.py (Diff revision 1)
     
     
     
     
     
     
     
    Show all issues
    Make sure these are alphabetical.
  3. rbtools/commands/rbattach.py (Diff revision 1)
     
     
     
     
    Show all issues
    No blank line here.
  4. rbtools/commands/rbattach.py (Diff revision 1)
     
     
    Show all issues
    For this and every other option, one parameter per line.
  5. rbtools/commands/rbattach.py (Diff revision 1)
     
     
     
     
    Show all issues
    Think I'd prefer this just be --caption
  6. rbtools/commands/rbattach.py (Diff revision 1)
     
     
    Show all issues
    Think I'd rather we just fix that function then. We shouldn't pollute this file. Instead of doing options.foo, you can in those cases do getattr(options, 'foo', somedefault).
  7. rbtools/commands/rbattach.py (Diff revision 1)
     
     
     
     
     
     
     
    Show all issues
    Curious, maybe this is just something I'm not aware of with the new command infrastructure, but why aren't these just provided as defaults when declaring the options above?
  8. rbtools/commands/rbattach.py (Diff revision 1)
     
     
    Show all issues
    Always treat these like proper sentences. It's missing a period.
  9. rbtools/commands/rbattach.py (Diff revision 1)
     
     
    Show all issues
    Trim the spaces.
  10. rbtools/commands/rbattach.py (Diff revision 1)
     
     
     
     
    Show all issues
    I agree. Let's do that.
  11. rbtools/commands/rbattach.py (Diff revision 1)
     
     
     
    Show all issues
    Pretty sure this fits on one line.
  12. rbtools/commands/rbattach.py (Diff revision 1)
     
     
     
     
     
    You're right, we should figure this out. Either in the base class or some utility function somewhere. I'd get smacleod's opinion on this too.
  13. rbtools/commands/rbattach.py (Diff revision 1)
     
     
     
    Show all issues
    Indentation is off.
  14. rbtools/commands/rbattach.py (Diff revision 1)
     
     
    Show all issues
    Things like this should be removed.
  15. rbtools/commands/rbattach.py (Diff revision 1)
     
     
    Show all issues
    The code's pretty self-explanatory. No need for the comment.
  16. rbtools/commands/rbattach.py (Diff revision 1)
     
     
    Show all issues
    Please use %s. This helps us localize.
  17. rbtools/commands/rbattach.py (Diff revision 1)
     
     
     
    Show all issues
    Maybe say what the default is.
  18. rbtools/commands/rbattach.py (Diff revision 1)
     
     
     
    Show all issues
    I can't imagine this looking right. Please properly terminate a string when you need to wrap and start it up again explicitly on the next line.
    
    I'd also change the text to:
    
    "Uploaded %s to review request %s."
  19. rbtools/commands/rbattach.py (Diff revision 1)
     
     
    Show all issues
    Alignment is off.
  20. 
      
JS
JS
SL
  1. 
      
  2. rbtools/commands/rbattach.py (Diff revision 3)
     
     
     
     
    Show all issues
    These should be alphabetical
  3. rbtools/commands/rbattach.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    You can probably condense:
    
    file_attachments = request.get_file_attachments()
    ...
    file_attachments.upload_attachment(filename, content,
                                       self.options.caption)
    
    To just:
    
    request.get_file_attachments().upload_attachment(filename, content,
                                                     self.options.caption)
    
  4. rbtools/commands/rbattach.py (Diff revision 3)
     
     
     
    Show all issues
    Should the the error be logged?
  5. 
      
JS
JS
JS
JS
chipx86
  1. 
      
  2. rbtools/commands/rbattach.py (Diff revision 6)
     
     
     
     
    Show all issues
    Two blank lines.
  3. rbtools/commands/rbattach.py (Diff revision 6)
     
     
    Show all issues
    Always should be proper sentences. Terminate with a period.
  4. rbtools/commands/rbattach.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues
    Are these needed?
  5. rbtools/commands/rbattach.py (Diff revision 6)
     
     
     
    Show all issues
    Same comments as your diff change. Can you go through those and apply those same changes to this function? That includes sys.exit.
    
    Kind of leaning toward moving this function into Client.
  6. rbtools/commands/rbattach.py (Diff revision 6)
     
     
    Show all issues
    Do we not want to use die()?
  7. rbtools/commands/rbattach.py (Diff revision 6)
     
     
     
    Show all issues
    Surely there may be errors. We need to check the status and report them.
  8. setup.py (Diff revision 6)
     
     
     
    Show all issues
    Alphabetical.
  9. 
      
mike_conley
  1. Updated patch?
  2. 
      
JS
mike_conley
  1. 
      
  2. rbtools/commands/rbattach.py (Diff revision 7)
     
     
     
    Show all issues
    I think I'd prefer:
    
    request = self.root_resource \
                  .get_review_requests() \
                  .get_item(request_id)
    1. Or, alternatively:
      
      request = \
          self.root_resource.get_review_requests().get_item(request_id)
    2. I'd prefer:
      
      review_requests = self.root_resource.get_review_requests()
      review_request = review_requests.get_item(request_id)
      
      That way we don't have to wrap, and it's clear what we're doing. Less chaining of stuff.
  3. rbtools/commands/rbattach.py (Diff revision 7)
     
     
     
    Show all issues
    I *think* this:
    
    request.get_file_attachments() \
           .upload_attachment(filename, content,
                              self.options.caption)
    
    Does PEP8 like that?
    1. Works for pep8. \ before period, gotta rememeber that for next time.
    2. How about:
      
      attachments = request.get_file_attachments()
      attachments.upload_attachment(...)
  4. rbtools/commands/rbattach.py (Diff revision 7)
     
     
    Show all issues
    Indented too much
  5. 
      
SL
  1. 
      
  2. rbtools/commands/rbattach.py (Diff revision 7)
     
     
     
     
     
     
     
    Show all issues
    I think you can do this with a Python "with" structure since RB 1.7 is moving to Python 2.5+; The key advantage is guaranteed file closure afterwards, no matter what happens.
    
    Something like the following:
    
    try:
        with open(path_to_file, 'r') as opened_file:
        content = opened_file.read()
    except IOError:
        die("%s is not a valid file." % (path_to_file))
    
    And the file closure is done for you.
    1. Oops, I messed up the indent, it should be:
      
      try:
          with open(path_to_file, 'r') as opened_file:
              content = opened_file.read()
      except IOError:
          die("%s is not a valid file." % (path_to_file))
    2. Cool idea, but I think I used a 'with open' before and I was told not to. I'll leave it as it is for now and if any of the mentors want the newer syntax, feel free to reopen.
    3. RB 1.7 will require 2.5 but rbtools will probably continue to support 2.4, at least for a little longer. We have less control over what individual users will have on their machines than we do over what people should install on their servers.
  3. 
      
JS
JS
david
  1. Your change description seems... odd. Your summary and diff is "RB Attach" but the description talks about "rb patch"
  2. 
      
JS
SM
  1. Ship It!
  2. 
      
JS
Review request changed
Status:
Completed
Change Summary:
Pushed to api (a6b0986940244692f0ee0570f2bac21a8101fb71).