Summary

Base command class and 'rb post' command.

Review Request #3449 — Created Oct. 24, 2012 and submitted Dec. 6, 2012, 11:28 a.m.

Information

Owner

smacleod

Repository

RBTools

Branch

api

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

This includes the beginnings of the new base class for rb commands,
and the rb post command.

'rb post' is meant to eventually replace post-review for the common
task of generating a diff and creating/updating a review request. It
has many of the extra options of post-review stripped out, to make
it more focused.

"rb <command>" now uses the new entry points, expecting subclasses
of ``Command``. If the command isn't found it will attempt to
execute "rb-<command>" on the system.

Testing Done

Local testing with git repositories. This review request was posted using 'rb post'

Issues

Description	From	Last Updated
Blank line between these. (Before/after if statements or other blocks when bunched up against code on the same indent level.)	chipx86	Oct. 26, 2012, 11:35 a.m.
Blank line between these.	chipx86	Oct. 26, 2012, 11:49 a.m.
Blank line between these.	chipx86	Oct. 26, 2012, 11:59 a.m.
Is that 'break' on the right indentation level?	chipx86	Oct. 26, 2012, 12:07 p.m.
Indentation looks off on the 2nd and 3rd line.	chipx86	Nov. 26, 2012, 11:52 a.m.
In this case, just use \ instead of parens.	chipx86	Oct. 26, 2012, 12:19 p.m.
Blank line.	chipx86	Oct. 26, 2012, 12:20 p.m.
Blank line.	chipx86	Oct. 26, 2012, 12:20 p.m.
Can just use \ in this case.	chipx86	Oct. 26, 2012, 12:20 p.m.
Probably should just do 'r/%s/' % .... That saves you the str() and is more readable.	chipx86	Oct. 26, 2012, 12:21 p.m.
Can you build the filename first? It'll make this less awkward.	chipx86	Oct. 26, 2012, 12:22 p.m.
Blank line should remain.	chipx86	Dec. 6, 2012, 11:27 a.m.

Some of my comments here may really be applying to older code that was just ported over. I'm not sure what's new and what isn't. But while you're here... :)

Overall this looks good. I mostly have some stylistic stuff (personal pet peeves of mine, mostly), with a few actually useful things to say.

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

The issue has been resolved. Show all issues

Blank line between these. (Before/after if statements or other blocks when bunched up against code on the same indent level.)

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

Do we want to handle the case where this throws some exception?

smacleod Oct. 26, 2012, 12:22 p.m.

Yeah, that's probably a good idea. I've added handling for when the entry point can't be loaded, and a catch all for any exceptions the command throws when initializing.

rbtools/commands/rb.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line between these.
```

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

We probably will want to change things such that stdout and stderr are not captured in this case, and instead are fed directly to the main stdout/stderr.

Possibly need to support stdin too.

SM

smacleod Oct. 26, 2012, 12:22 p.m.
```
Definitely. I've added a TODO for now.
```

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

Wondering if we should put "The Review Board Project" as the author for all first-party commands, to better distinguish from third-party commands.

smacleod Oct. 26, 2012, 12:22 p.m.

Yeah, it will be a little strange if all the commands have various authors tagged.

rbtools/commands/rbpost.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line between these.
```

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

The issue has been resolved. Show all issues

Is that 'break' on the right indentation level?

smacleod Oct. 26, 2012, 12:22 p.m.

Ah thanks for pointing this out. There's actually a bigger problem here, instead
of breaking I probably want to raise a StopIteration, to break out of both loops.
It, of course, should be moved up into the if (where the break should have been).

(Also, self.info needed to be changed to self.repository_info :/)

The code in post-review this is derived from actually has a bug itself; it's only
grabbing a single page of repositories. So, if there are more than 25 repos, it
won't check all of them. I fixed this for rb post, but do you think it's worth
patching in post-review?

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

The issue has been resolved. Show all issues

Indentation looks off on the 2nd and 3rd line.

smacleod Oct. 26, 2012, 12:22 p.m.

What would you like to see here for indentation? I was going for something like this:

review_request = self.api_root.get_review_request(
    values={
        'review_request_id': self.options.rid,
    })

but with the 'values={' moved up. Should it be:

review_request = self.api_root.get_review_request(values={
    'review_request_id': self.options.rid,
})

chipx86 Oct. 27, 2012, 2:48 p.m.

Either of those are fine (though in this case, I'd pick the latter). It's just weird when values= is on the first line but then }) isn't aligned with the start of that line.

rbtools/commands/rbpost.py (Diff revision 1)
The issue has been resolved. Show all issues
```
In this case, just use \ instead of parens.
```
rbtools/commands/rbpost.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line.
```
rbtools/commands/rbpost.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Blank line.
```
rbtools/commands/rbpost.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can just use \ in this case.
```

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

The issue has been resolved. Show all issues

Probably should just do 'r/%s/' % .... That saves you the str() and is more readable.

rbtools/commands/rbpost.py (Diff revision 1)
The issue has been resolved. Show all issues
```
Can you build the filename first? It'll make this less awkward.
```

Change Summary:

Fixes issues from Christian's review and update based on changes to the api.

Summary:

Draft base command class and 'rb post' command.

Base command class and 'rb post' command.

Description:

~		This includes a draft of the new base class for rb commands, and the
~		rb post command.
	~	This includes the beginnings of the new base class for rb commands,
	~	and the rb post command.

		'rb post' is meant to eventually replace post-review for the common
		task of generating a diff and creating/updating a review request. It
		has many of the extra options of post-review stripped out, to make
		it more focused.

		"rb <command>" now uses the new entry points, expecting subclasses
		of `Command`. If the command isn't found it will attempt to
		execute "rb-<command>" on the system.

Diff:

Revision 2 (+692 -69)

Show changes

	setup.py
	rbtools/commands/__init__.py
	rbtools/commands/rb.py
	rbtools/commands/rbpost.py
	rbtools/utils/filesystem.py

Ship it!

setup.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Blank line should remain.
```

Status:: Completed
Change Summary:: Pushed to api (4861799146).