Summary

Use "rbt" instead of "rb" for base command.

Review Request #3727 — Created Jan. 6, 2013 and submitted Jan. 9, 2013, 6:20 p.m.

Information

Owner

smacleod

Repository

RBTools

Branch

api

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

This work is on top of John Sintal's work on several commands. Once
This has been reviewed, both will be pushed together. The command
files have not been modified, just renamed.

Use "rbt" instead of "rb" for base command.

The base command "rb" conflicted with another package, so to avoid
problems we will now use "rbt" instead. I refactored things a bit to
make changing this name easier in the future.

Add Steven MacLeod and John Sintal to AUTHORS


Remove unused files.

This removes a number of unused command and api files which have been
sitting around. It's possible some of the logic present in the command
files could be useful in the future, but the implementation details
would need to be changed drastically.

Testing Done

Issues

Description	From	Last Updated
Can you use 'f' instead of 'opened_file"? It's more common.	chipx86	Jan. 8, 2013, 9:21 a.m.
Two blank lines.	chipx86	Jan. 8, 2013, 9:22 a.m.
The """ should be on the next line.	chipx86	Jan. 8, 2013, 9:22 a.m.
Is there any need to store these in self?	chipx86	Jan. 8, 2013, 9:25 a.m.
Can just be: if not diff: Or maybe just "if diff" and reverse the two clauses.	chipx86	Jan. 8, 2013, 9:28 a.m.
It's possible that an ImportError is due to something successfully imported being unable to import something else. A stack trace …	chipx86	Jan. 8, 2013, 10:35 a.m.
This feels like a hack, and possibly a bad assumption. We should specifically get the latest diff, not assume total_results …	chipx86	Jan. 8, 2013, 10:24 a.m.
We should call diffs.get_item(patch_id) only once.	chipx86	Jan. 8, 2013, 10:24 a.m.
Too much of a mix of imports/froms	chipx86	Jan. 8, 2013, 10:28 a.m.
Col: 27 E127 continuation line over-indented for visual indent	reviewbot	Jan. 6, 2013, 8:03 p.m.
Probably no need to say "the web interface," because the command line would suffice too.	chipx86	Jan. 8, 2013, 10:29 a.m.
Should explicitly say "review request" instead of "request"	chipx86	Jan. 8, 2013, 10:30 a.m.
Col: 23 E127 continuation line over-indented for visual indent	reviewbot	Jan. 6, 2013, 8:13 p.m.
Look at the equivalent code in post-review. I believe that this code was just recently changed to handle some stdout/stderr …	chipx86	Jan. 8, 2013, 10:25 a.m.
The 'pass' is not needed.	chipx86	Jan. 8, 2013, 10:30 a.m.
Any reason to store in self?	chipx86	Jan. 8, 2013, 10:31 a.m.
Col: 27 E127 continuation line over-indented for visual indent	reviewbot	Jan. 8, 2013, 11:11 a.m.
Col: 23 E127 continuation line over-indented for visual indent	reviewbot	Jan. 8, 2013, 11:11 a.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/commands/main.py
    rbtools/commands/__init__.py
    rbtools/commands/close.py
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/commands/attach.py
    rbtools/commands/publish.py
    setup.py
    rbtools/commands/diff.py
  Ignored Files:
    rbtools/api/settings.py
    rbtools/api/serverinterface.py
    rbtools/commands/rbinfo.py
    rbtools/commands/rbinfov2.py
    rbtools/knownissues.txt
    rbtools/commands/rbattach.py
    rbtools/commands/rbupload.py
    rbtools/commands/rb.py
    rbtools/commands/utils.py
    rbtools/commands/tests.py
    rbtools/commands/rbdiff.py
    rbtools/commands/rbcreate.py
    rbtools/commands/rbconfig.py
    rbtools/commands/rbpatch.py
    AUTHORS
    rbtools/commands/rbpublish.py
    rbtools/commands/resource-browser.py
    rbtools/commands/rbopen.py
    rbtools/commands/rbclose.py
    rbtools/commands/rbpost.py

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

Show all issues

Col: 27
 E127 continuation line over-indented for visual indent

smacleod Jan. 6, 2013, 8:03 p.m.

This is fixed in the review request which comes after.

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

Show all issues

Col: 23
 E127 continuation line over-indented for visual indent

Ship it!

```
Ship It!
```

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

Show all issues

Can you use 'f' instead of 'opened_file"? It's more common.

rbtools/commands/close.py (Diff revision 1)
Show all issues
```
Two blank lines.
```
rbtools/commands/close.py (Diff revision 1)
Show all issues
```
The """ should be on the next line.
```
rbtools/commands/close.py (Diff revision 1)
Show all issues
```
Is there any need to store these in self?
```

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

Show all issues

Can just be:

if not diff:


Or maybe just "if diff" and reverse the two clauses.

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

Show all issues

It's possible that an ImportError is due to something successfully imported being unable to import something else. A stack trace might be helpful here.

We may want to use Python logging for such things.

smacleod Jan. 8, 2013, 10:35 a.m.

Errors and output is being handled in different ways all over the place (calls to sys.stderr.write, die, print, sys.exit etc.). I'm going to do a sweep to standardize things with logging, and post a new review request for it. I've added a comment here to remind me about the strack trace though.

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

Show all issues

This feels like a hack, and possibly a bad assumption. We should specifically get the latest diff, not assume total_results is numerically the same as the latest ID.

smacleod Jan. 8, 2013, 9:51 a.m.

I *think* we're safe here, but I can see why you thought we
weren't, I thought it was wrong as well for a couple minutes.

If you rename 'patch_id' to 'patch_revision' (which is what
that variable is actually representing), it should look
correct (diffs use the revision as their identifier in the
api urls, and thus the following 'get_item' call will work).

So, with this in mind, the actual assumption being made here
is that the api items are sorted in ascending order by diff
revision, and no number is missing from the list, which
appears to be true. In order to not use this assumption, we
would have to iterate over every diff, and keep track of the
max revision. So I think we're fine to use this assumption.

That being said, the variable naming used here is definitely
confusing, and assumptions like this should be commented. I'll
rework this section a bit and make it more clear.

Thanks for bringing this up.

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

Show all issues

We should call diffs.get_item(patch_id) only once.

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

Show all issues

Too much of a mix of imports/froms

SM

smacleod Jan. 8, 2013, 10:35 a.m.
```
This is fixed in /r/3726
```

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

Show all issues

Probably no need to say "the web interface," because the command line would suffice too.

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

Show all issues

Should explicitly say "review request" instead of "request"

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

Show all issues

Look at the equivalent code in post-review. I believe that this code was just recently changed to handle some stdout/stderr issues.

smacleod Jan. 8, 2013, 10:26 a.m.

I'm going to deal with all the post-review bug fixes when I merge master into api. This will be coming in another review request.

rbtools/commands/post.py (Diff revision 1)
Show all issues
```
The 'pass' is not needed.
```
rbtools/commands/publish.py (Diff revision 1)
Show all issues
```
Any reason to store in self?
```

Change Summary:

Updated based on Christian's review

Diff:

Revision 2 (+991 -2455)

Show changes

	AUTHORS
	setup.py
	rbtools/knownissues.txt
	rbtools/api/serverinterface.py
	rbtools/api/settings.py
	rbtools/commands/__init__.py
	rbtools/commands/attach.py
	rbtools/commands/close.py
	21 more

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    rbtools/commands/main.py
    rbtools/commands/__init__.py
    rbtools/commands/diff.py
    rbtools/commands/patch.py
    rbtools/commands/post.py
    rbtools/commands/attach.py
    rbtools/commands/publish.py
    setup.py
    rbtools/commands/close.py
  Ignored Files:
    rbtools/api/serverinterface.py
    rbtools/api/settings.py
    rbtools/commands/rbinfo.py
    rbtools/commands/rbinfov2.py
    rbtools/knownissues.txt
    rbtools/commands/rbattach.py
    rbtools/commands/rbupload.py
    rbtools/commands/rb.py
    rbtools/commands/utils.py
    rbtools/commands/tests.py
    rbtools/commands/rbdiff.py
    rbtools/commands/rbcreate.py
    rbtools/commands/rbconfig.py
    rbtools/commands/rbpatch.py
    AUTHORS
    rbtools/commands/rbpublish.py
    rbtools/commands/resource-browser.py
    rbtools/commands/rbopen.py
    rbtools/commands/rbclose.py
    rbtools/commands/rbpost.py

rbtools/commands/post.py (Diff revision 2)

Show all issues

Col: 27
 E127 continuation line over-indented for visual indent

rbtools/commands/post.py (Diff revision 2)

Show all issues

Col: 23
 E127 continuation line over-indented for visual indent

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Merged into api (2b12805a6b392e85f4a6e21424e0628736c1b405).