- Download Diff

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)

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)
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)
Show all issues
```
Blank line between these.
```

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

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)

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)
Show all issues
```
In this case, just use \ instead of parens.
```
rbtools/commands/rbpost.py (Diff revision 1)
Show all issues
```
Blank line.
```
rbtools/commands/rbpost.py (Diff revision 1)
Show all issues
```
Blank line.
```
rbtools/commands/rbpost.py (Diff revision 1)
Show all issues
```
Can just use \ in this case.
```

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

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)

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)
Show all issues
```
Blank line should remain.
```

Status: Closed (submitted)

Change Summary:

Pushed to api (4861799146).

You have a pending review.

Review Board 5.0.2 alpha 0 (dev)

Base command class and 'rb post' command.

Change Summary:

Summary:

Description:

Diff:

Status: Closed (submitted)

Change Summary: