This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/cmdline/rbsite.py
  Ignored Files:

Col: 1
 E302 expected 2 blank lines, found 1

Col: 80
 E501 line too long (82 > 79 characters)

I'd prefer "reviewboard" instead of "ReviewBoard."

Also, there should be two blank lines here.

Should be one line.

You can get rid of "A class that"

Sentences in comments should end with a period. Same goes with other comments in this change.

Instead of a dict, this should probably just be a set().

For consistency, can you explicitly add the ", 'r'" ?

Blank line before this.

Blank line before/after loops and other blocks.

This won't work on Python 2.5, which we still support.

Where is this set? I only see it below, but it doesn't look like it'd be accessible here.

Also, should be is_win.

Should be "is_win", or better, just collapse this with the conditional and don't use a variable.

Blank line after this.

Also, when possible, use single quotes instead of double. Looks cleaner.

Blank line above.

This can just be print.

Like above, we can't use format(). Should use the old-style formatting arguments.

Indentation is funky.

The "or" should be on the previous line, and the "(len(args) ..." should be aligned with the "args[0] not in ..."

Can this be combined with the check below? Something like:


if len(args) == 2 and command in COMMANDS:
    install_dirs = [args[1]]
elif len(args) < 2 and command == 'upgrade':
    sitelist = ...
    install_dirs = ...
else:
    print_help

Should be one line, like:

"""Maintains blah blah."""

We generally prefer more explicit variables, like ordered_sites.

This can just be "w", since we don't need to read from it.

Blank line after.

sites is already public, and is fine to use. We don't need this method.

It seems we still benefit from having this, since we're doing the Windows comparison twice.

(Although, while you're here, is_windows would be more correct.)

This would be 'if not isWin'

Should use != instead of not.

I'm not sure users need to see this every time. It's really an internal detail. (If we were using logging in here, I'd suggest logging.debug).

Just a thought.. Any harm in letting this accept variable arguments, now that upgrade supports upgrading multiple sites?

Probably only makes sense for the upgrade command.

The one thing I'm trying to figure out is whether we want this to happen automatically, or whether we want to require an option (say, --all-sites).

Right now, doing 'rb-site install' or 'rb-site upgrade' will give you usage information on them. This code changes behavior to now make 'rb-site upgrade' *maybe* give you usage information, but *probably* upgrade all your sites.

I'm leaning toward wanting an --all-sites option for this, so that the behavior is explicit. That also gives us the ability to give a meaningful error if that was requested but no sites were found (such as for existing installs that don't have an entry, or for Windows).

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/cmdline/rbsite.py
  Ignored Files:

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/cmdline/rbsite.py
  Ignored Files:

This will fail if /etc/reviewboard doesn't already exist. We should attempt to make the path, and log an error if we can't.

Blank line before this.

Is this related to the change? If not, it should probably be left, or documented with a comment.

My only concern is that, while it's probably a correct change, it does have the potential to break things if anything relies upon the admin's home directory at all.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/cmdline/rbsite.py
  Ignored Files:

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/cmdline/rbsite.py
  Ignored Files:

 undefined name 'error'

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/cmdline/rbsite.py
  Ignored Files:

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/cmdline/rbsite.py
  Ignored Files:

Ship It!