According to PEP-8 (the python style guide), imports should be separated into three groups, with a blank line in between each of those groups.

Standard library (os, random, optparse)

Other 3rd party modules (django)

Package imports (reviewboard)



Within each of these three sections, please alphabetize the import lines.

Trailing space.

The help text here is a little repetitive. I'd get rid of "Specifies the" from each, and provide examples of what the input should look like, especially for the range items.

I think these would be more helpful:
"The number of users to add"
"The number of review requests per user [min:max]"
"The number of diffs per user [min:max]"

I also think that "diffs" should be per review request, and "diff-comments" should be per diff.

Also your last help text (for diff-comments) is incorrect.

It might be nice to let people either specify a single number or a range (you can parse that fairly easily). These might also be nicer to store as tuples instead of separate variables for min and max.

I know that a lot of this kind of output is probably temporary and for debug purposes, but chances are pretty good that the user knows what they entered. The output should inform about the progress and communicate how long the user has to wait before long-running operations are done.

Trailing space.

This can be much shorter:

return (int(item.strip()) for item in com_string.split(':'))

Also note that we wrap lines to 80 characters.

Since this is an error message, please print it to stderr.

print >> sys.stderr, "..."

Also wrap to 80 characters.

Ship It!

Steve:

Good work on this.

There are a few minor style things - a sweep with the pep8 script (http://pypi.python.org/pypi/pep8) will help you nail them all.

Some other concerns:
1)  Shouldn't we create the reviews/management/diffs folder, instead of asking users to create it on their own?
2)  Shouldn't we include some .diff files, instead of asking users to create them on their own?
3)  According to the Django documentation, @transaction.commit_on_success only rolls back if we throw an exception.  In some cases, you're exiting on a failure, which means that the database commits the transaction at that point - even though we've failed.  This can leave the database in a tricky state (I got this error after not having any .diff files, and it wouldn't go away until I reverted by DB manually:  http://www.pastie.org/1667934).  Better to throw exceptions, and rollback the DB on failure.

So those are my comments.  Thanks for your work,

-Mike

Two blank lines here.

Space between "#" and "Generate"

Why is manage.py here?  I think you can just have "/../scmtools/testdata/git_repo" and get the same effect.

Similarly, I don't think manage.py is required here.

Hold up - so we need to add a new folder called "diffs" under "reviews/management/commands/diffs"?  That should probably be part of this review request too, then.

Git generally ignores empty folders, but you can get it to pay attention by putting an empty .gitignore in it.

At least two spaces before inline comment, and a space between "#" and "add"

Hm...and we need .diff files too?

Should we generate these ourselves, and include them?

Since you're using @transaction.commit_on_success, you'll probably want to throw exceptions to ensure that the db gets rolled back.

Space on either side of +

Two spaces before inline comment, and space between "#" and "avoids"

Or should this be a fill-database optional parameter?

Space on either side of "=" operator.

There are a bunch of these, but I'll just mention it this one time.

I think these should be different for each review request - it'll give a better sense of variety when looking at the dashboard, instead of seeing the same summary/descriptions all over the place.  Maybe some Lorem Ipsum would be appropriate here.

I think these comments are OK to be in lowercase.  ;)

Line exceeds 80 chars

So you just want the most recent one?

file_diff = cur_diff.files.order_by('timestamp')[0]

oughta do the trick, unless I misunderstand what you're trying to do.

Also, two spaces before inline comment, space after "#".

Space after #, lowercase the comment.

Hm.  We should guarantee that the diffs we provide (if we're providing diffs) have more than 220 lines.

Or, like you say, we need to detect that per diff.  Hrm.  Is there no diffviewer util for this?

space after , and space on either side of -

space after ,

Hm.  NameError is for when a local or global variable is not defined.  Both output and req_val *should* be defined here...not sure why you need to catch this one, nor pass it silently.

Maybe provide an example here - like "Example:  --review-requests=2:5"

Again, we'll want to throw an exception in order to rollback the db.

More concise / neater option:

return ''.join(random.choice(string.ascii_lowercase) for x in range(5, 8))

I'd prefer failing early, like:

if not value:
  return 0

if len(value) == 1:
  return value[0]

return random.randrange(value[0], value[1])

You can import both of these at once.

forms, and then models (alphabetical order on the module level)

You probably should do:

def handle_noargs(self, users=None, review_requests=None, ....., **options):

Most management commands will take a verbosity parameter. That should probably be used. That way things can be done from scripts.

Parameters should be aligned.

Use os.path.join:

repo_dir = os.path.join(os.path.abspath(sys.argv[0]), "..", "scmtools", "testdata", "git_repo")

This ensures it works across platforms.

This is an odd way to do it. I don't really think oyu need to throw an exception and then catch it immediately. Just do the if statement and then print the error.

This should actually be:

raise CommandError('...')

No trailing \n.

Also, that lets you get rid of the exit() below. It means that other commands can wrap this and not terminate early.

Probably should combine to one statement.

Comments should be in sentence casing with trailing periods.

Same thing about os.path.join

Same about comments. Here and everywhere else.

Same comments as above.

Shouldn't need to? Especially if using os.path.join.

Space after #

This can be simplified as:

files = [f for f in os.listdir(diff_dir)
         if f.endswith('.diff')]

Same comments about comment styles.

Same as above. Just going to assume you'll fix these for the others below.

Parameters should align.

One per line.

We should have different names so we can distinguish when looking at pages.

Should use @example.com, since @email.com might be legit (and @example.com is allowed by some RFC)

Rather than this, we should always use set_password.

The SHA1 isn't good enough, because it depends on the SECRET_KEY.

One per line.

You should be able to leave these out.

One per line.

Should use print, and one per actual line.

Also, instead of casting to strings and concatenating, use Python format strings.

No blank line.

print "Request #%s:" % j

Params should align.

I think None can fit on the same line though?

User.objects.get(pk=1)

Actually, we're doing this in a loop, so let's only fetch this one time throughout the whole lifetime of this program. It'll speed things up a bit.

Format strings.

No blank line.

Same comments about format strings and printing.

'filename' is usually a string. Use 'f'

Should be able to fit on a line./

No blank line.

Same as above. I'm going to assume you'll go through and take care of the rest.

Can these params not fit after .create( ?

Should do this as infrequently as possible.

Format strings.

Kind of icky. Do we really need to do that so often? The inner loop isn't good enough?

Should use print for each logical line. That'd be a total of two.

Also, format strings.

Descriptions of functions should be in python docstring format:

def parseCommand(...)
    """One line summary"""

or:

    """One line summary

    Optional description.
    """

Params should align.

No blank line.

I don't like magic numbers. These should be constants near the top of the file.

We're going to create this every single time this is called, which will be slow. Define it as a constant at the top of the file.

Params should be aligned.

Make sure master is merged in and references were fetched from origin. I'm seeing stray changes in here.

Also, best to keep this just "Fill-database script" or something in the summary. Things shouldn't have a version number until they're officially in, and changing the summary breaks the threading in my e-mail client :)

Once this is updated with master, I'll do another review.

The code looks good, but the testdata/git_repo isn't contained in your patch because it's binary. Can you send me a tar archive of that directory?

Pushed to master as 4e22a3d. Thanks!