Summary

RB Attach

Review Request #3599 — Created Dec. 1, 2012 and submitted Jan. 9, 2013, 6:19 p.m.

Information

Owner

jsintal

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

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)

Testing Done

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)

Issues

Description	From	Last Updated
Make sure these are alphabetical.	chipx86	Dec. 3, 2012, 10:39 p.m.
No blank line here.	chipx86	Dec. 3, 2012, 10:39 p.m.
For this and every other option, one parameter per line.	chipx86	Dec. 4, 2012, 7:17 p.m.
Think I'd prefer this just be --caption	chipx86	Dec. 2, 2012, 2:05 p.m.
Think I'd rather we just fix that function then. We shouldn't pollute this file. Instead of doing options.foo, you can …	chipx86	Dec. 3, 2012, 10:40 p.m.
Curious, maybe this is just something I'm not aware of with the new command infrastructure, but why aren't these just …	chipx86	Dec. 2, 2012, 2:05 p.m.
Always treat these like proper sentences. It's missing a period.	chipx86	Dec. 3, 2012, 10:40 p.m.
Trim the spaces.	chipx86	Dec. 2, 2012, 2:05 p.m.
I agree. Let's do that.	chipx86	Dec. 2, 2012, 2:05 p.m.
Pretty sure this fits on one line.	chipx86	Dec. 2, 2012, 2:06 p.m.
Indentation is off.	chipx86	Dec. 2, 2012, 2:13 p.m.
Things like this should be removed.	chipx86	Dec. 2, 2012, 2:06 p.m.
The code's pretty self-explanatory. No need for the comment.	chipx86	Dec. 2, 2012, 2:06 p.m.
Please use %s. This helps us localize.	chipx86	Dec. 2, 2012, 2:07 p.m.
Maybe say what the default is.	chipx86	Dec. 2, 2012, 2:13 p.m.
I can't imagine this looking right. Please properly terminate a string when you need to wrap and start it up …	chipx86	Dec. 2, 2012, 2:13 p.m.
Alignment is off.	chipx86	Dec. 2, 2012, 2:13 p.m.
These should be alphabetical	SL slchen	Dec. 3, 2012, 10:36 p.m.
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	Dec. 4, 2012, 7:18 p.m.
Should the the error be logged?	SL slchen	Dec. 3, 2012, 10:39 p.m.
Two blank lines.	chipx86	Dec. 4, 2012, 3:55 p.m.
Always should be proper sentences. Terminate with a period.	chipx86	Dec. 4, 2012, 3:55 p.m.
Are these needed?	chipx86	Dec. 4, 2012, 3:57 p.m.
Same comments as your diff change. Can you go through those and apply those same changes to this function? That …	chipx86	Dec. 4, 2012, 3:59 p.m.
Do we not want to use die()?	chipx86	Dec. 4, 2012, 4 p.m.
Surely there may be errors. We need to check the status and report them.	chipx86	Dec. 4, 2012, 4:18 p.m.
Alphabetical.	chipx86	Dec. 4, 2012, 4:12 p.m.
I think I'd prefer: request = self.root_resource \ .get_review_requests() \ .get_item(request_id)	mike_conley	Dec. 4, 2012, 8:50 p.m.
I think you can do this with a Python "with" structure since RB 1.7 is moving to Python 2.5+; The …	SL slchen	Dec. 4, 2012, 8:43 p.m.
I think this: request.get_file_attachments() \ .upload_attachment(filename, content, self.options.caption) Does PEP8 like that?	mike_conley	Dec. 4, 2012, 8:49 p.m.
Indented too much	mike_conley	Dec. 4, 2012, 8:48 p.m.

Is this rb attach, or rb patch? The summary and other fields disagree.

rbtools/commands/rbattach.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Make sure these are alphabetical.
```
rbtools/commands/rbattach.py (Diff revision 1)
The issue has been resolved. Show all issues
```
No blank line here.
```
rbtools/commands/rbattach.py (Diff revision 1)
The issue has been resolved. Show all issues
```
For this and every other option, one parameter per line.
```
rbtools/commands/rbattach.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Think I'd prefer this just be --caption
```

rbtools/commands/rbattach.py (Diff revision 1)

The issue has been resolved. 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).

rbtools/commands/rbattach.py (Diff revision 1)

The issue has been resolved. 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?

rbtools/commands/rbattach.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Always treat these like proper sentences. It's missing a period.
```
rbtools/commands/rbattach.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Trim the spaces.
```
rbtools/commands/rbattach.py (Diff revision 1)
The issue has been resolved. Show all issues
```
I agree. Let's do that.
```
rbtools/commands/rbattach.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Pretty sure this fits on one line.
```

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.

rbtools/commands/rbattach.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Indentation is off.
```
rbtools/commands/rbattach.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Things like this should be removed.
```
rbtools/commands/rbattach.py (Diff revision 1)
The issue has been resolved. Show all issues
```
The code's pretty self-explanatory. No need for the comment.
```
rbtools/commands/rbattach.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Please use %s. This helps us localize.
```
rbtools/commands/rbattach.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Maybe say what the default is.
```

rbtools/commands/rbattach.py (Diff revision 1)

The issue has been resolved. 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."

rbtools/commands/rbattach.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Alignment is off.
```

Change Summary:

- refactored according to the changes in base commadnd
- pep8 style fixes

Diff:

Revision 2 (+115)

Show changes

	setup.py
	rbtools/commands/rbattach.py

Change Summary:

Changed the way commands are branched from other features (rb-general). Ex.: http://pastie.org/5468764

Diff:

Revision 3 (+115)

Show changes

	setup.py
	rbtools/commands/rbattach.py

rbtools/commands/rbattach.py (Diff revision 3)
The issue has been resolved. Show all issues
```
These should be alphabetical
```

rbtools/commands/rbattach.py (Diff revision 3)

The issue has been resolved. 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)

rbtools/commands/rbattach.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Should the the error be logged?
```

Change Summary:

rebased on rb-general

Testing Done:

		Tried the following on dev server, all of which were successful:

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

		rb patch <invalid_request_id>
		rb patch <invalid_request_id> <invalid/path/to/file>
		rb patch <invalid/path/to/file>
	+
	+	Retested with rb-general (removal of extra options)

Diff:

Revision 4 (+115)

Show changes

	setup.py
	rbtools/commands/rbattach.py

Change Summary:

import fix, removed options

Diff:

Revision 5 (+82)

Show changes

	setup.py
	rbtools/commands/rbattach.py

Change Summary:

Rebased on using the gettatr(options, field, default method for ALL

Diff:

Revision 6 (+82)

Show changes

	setup.py
	rbtools/commands/rbattach.py

Description:

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

		Args: rb patch <request_id> <path/to/file>
		Options: --description (caption), --filename (custom filename)
-
-		TODO:
-		refactor
-		superfluous options

rbtools/commands/rbattach.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Two blank lines.
```
rbtools/commands/rbattach.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Always should be proper sentences. Terminate with a period.
```

rbtools/commands/rbattach.py (Diff revision 6)

The issue has been resolved. Show all issues

Are these needed?

rbtools/commands/rbattach.py (Diff revision 6)

The issue has been resolved. 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.

rbtools/commands/rbattach.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Do we not want to use die()?
```
rbtools/commands/rbattach.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Surely there may be errors. We need to check the status and report them.
```
setup.py (Diff revision 6)
The issue has been resolved. Show all issues
```
Alphabetical.
```

```
Updated patch?
```

Change Summary:

Rebased on using the gettatr(options, field, default method for ALL

Diff:

Revision 7 (+76)

Show changes

	setup.py
	rbtools/commands/rbattach.py

rbtools/commands/rbattach.py (Diff revision 7)

The issue has been resolved. Show all issues

I think I'd prefer:

request = self.root_resource \
              .get_review_requests() \
              .get_item(request_id)

mike_conley Dec. 4, 2012, 8:24 p.m.

Or, alternatively:

request = \
    self.root_resource.get_review_requests().get_item(request_id)

chipx86 Dec. 4, 2012, 10:31 p.m.

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.

rbtools/commands/rbattach.py (Diff revision 7)

The issue has been resolved. Show all issues

I *think* this:

request.get_file_attachments() \
       .upload_attachment(filename, content,
                          self.options.caption)

Does PEP8 like that?

jsintal Dec. 4, 2012, 8:49 p.m.

Works for pep8. \ before period, gotta rememeber that for next time.

chipx86 Dec. 4, 2012, 10:31 p.m.

How about:

attachments = request.get_file_attachments()
attachments.upload_attachment(...)

rbtools/commands/rbattach.py (Diff revision 7)
The issue has been resolved. Show all issues
```
Indented too much
```

rbtools/commands/rbattach.py (Diff revision 7)

The issue has been dropped. 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.

slchen Dec. 4, 2012, 8:27 p.m.

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))

jsintal Dec. 4, 2012, 8:43 p.m.

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.

david Dec. 4, 2012, 10:26 p.m.

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.

Change Summary:

Addressed all issues

Diff:

Revision 8 (+76)

Show changes

	setup.py
	rbtools/commands/rbattach.py

Change Summary:

Minor bug fix

Diff:

Revision 9 (+77)

Show changes

	setup.py
	rbtools/commands/rbattach.py

Your change description seems... odd. Your summary and diff is "RB Attach" but the description talks about "rb patch"

Change Summary:

patch --> attach

Description:

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

~		Args: rb patch <request_id> <path/to/file>
	~	Args: rb attach <request_id> <path/to/file>
		Options: --description (caption), --filename (custom filename)

Testing Done:

		Tried the following on dev server, all of which were successful:

~		rb patch <request_id> <path/to/file>
~		rb patch <request_id> <path/to/file> --description='some desc'
~		rb patch <request_id> <path/to/file> --filename='custom filename'
~		rb patch <request_id> <path/to/file> --description='some desc' --filename='custom filename'
	~	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 patch <invalid_request_id>
	~	rb attach <invalid_request_id>
		rb patch <invalid_request_id> <invalid/path/to/file>
~		rb patch <invalid/path/to/file>
	~	rb attach <invalid/path/to/file>

		Retested with rb-general (removal of extra options)

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to api (a6b0986940244692f0ee0570f2bac21a8101fb71).