Some common style stuff to fix up.

Before we add new fields to the database, I want to be sure we're absolutely going to use them, and that those are more or less set in stone, or it'll be harder to migrate away from it later.

It'd also help if there were unit tests for the new bits of logic.

Trailing whitespace.

Make sure these are all < 80 chars in length.

Do we know that this field is necessary?

Two blank lines between sections.

The imports must be in alphabetical order.

< 80 columns.

Ideally, one parameter to filter per line.

Trailing whitespace.

Can you go through the rest of the file and fix those?

< 80 chars.

Make sure all the HTML uses 1 space indenting.

Indent any {% %} that controls logic flow (for loops, if statements) at indent level 0.

Same with the ones below.

Jacob:

Thanks for your work here - just some whitespace that needs to be cleared, and I'm wondering about a magic number that's shown up.

There were two issues that Christian brought up in his last review that haven't been addressed, it seems.  Could you go back and fix those, or comment on why they don't need to be addressed?

Also, could you attach a screenshot showing what this new bit to the diff fragment viewer looks like?

Thanks,

-Mike

White space here too, and below on line 7.

White space

Where is the magic number 9 coming from?

Hm - this could get confusing - there's already a "file" being used outside this loop scope.  Is there another variable name you could use instead?

Jacob:

This is looking pretty good - just a few minor notes, thanks.

-Mike

Whitespace in this file too!

whitespace

I'm a bit wary of having the magic number 9 around.  It should probably be assigned to a constant, or at the very least, there should be a comment here explaining what the 9 is all about.

Hey Jacob:

Just a few more formatting / style notes.  I've found some lines where the indenting feels a little awkward, but I'm not 100% what the resolution is...maybe Christian, David, or one of the other developers have some input?

Thanks for your work,

-Mike

I think we'll want this line indented enough so that the "m" of "max_length" is directly below the "_" of "_(source file revision".

That way, it's easier to tell that those items are within the parentheses following CharField.

I feel a bit funny about this indentation.  Either the diff_source_revision line should be lined up directly underneath the "diff_source_file" parameter...or, if that causes us to be > 80 chars....I'm not sure.

Not sure what the solution would be there.  :/  At the very least, though, when in doubt, the indenting should be 4 spaces.

Anyone have any input on what the right style would be here?

Constant's are generally assigned to variables in all caps, so this should be:

REVISION_PREFIX_COUNT

Again, I feel funny about this indentation.  I understand that we want to get these lines < 80 chars, but it makes it harder to tell that these diff_source_x params are within the parentheses following filter.

Christian / David:  do you have a preference?

Same as above.

While I'm not going to ask you to get rid of all of the trailing whitespace in this file, I don't think it's good to add to it either.  The trailing whitespace should be removed from these lines.

Indent a single space.

Maybe wrap entire div with the ifequal statement rather than try to hide the div if the count 0.

Add a single indent.

Use class clearfix for this.