E251 unexpected spaces around keyword / parameter equals

E251 unexpected spaces around keyword / parameter equals

E128 continuation line under-indented for visual indent

E128 continuation line under-indented for visual indent

E128 continuation line under-indented for visual indent

E128 continuation line under-indented for visual indent

E501 line too long (94 > 79 characters)

E124 closing bracket does not match visual indentation

E501 line too long (94 > 79 characters)

E501 line too long (84 > 79 characters)

E225 missing whitespace around operator

W291 trailing whitespace

E501 line too long (84 > 79 characters)

E251 unexpected spaces around keyword / parameter equals

E251 unexpected spaces around keyword / parameter equals

W291 trailing whitespace

W291 trailing whitespace

It might be nicer to do elif six.PY2 and then an else instead of having nested if else statements

F841 local variable 'diff' is assigned to but never used

E303 too many blank lines (2)

W292 no newline at end of file

Instead of just having this be a boolean, I think we should follow git's lead and have it be a …

F841 local variable 'diff' is assigned to but never used

Perhaps we should call this "display options" to distinguish it from Command.diff_options?

Add a blank line between these two.

This needs to tell us what the type is.

Add a blank line between these two.

Please put this blank line back.

Indentation is a little bit funky here and things don't quite line up with the parens. I think it might …

All test docstrings should start with the word "Testing" for consistency in output.

Please add a blank line between these two

E999 SyntaxError: invalid syntax

Please add a blank line between these two

There's an extra space at the end of this docstring.

Let's use single quotes instead of double here.

This should be (list of unicode, optional)

I don't think this adds useful info--the first line of explanation is sufficient.

DIFF is incorrectly capitalized here.

Please add a blank line between these two.

This needs a docstring. It probably should also be prefixed with _

Please add a blank line between these two.

These should be grouped together with full-package imports coming before from imports. Please also alphabetize them (formatters before lexers). import …

Let's give this method an imperative name. Perhaps colorize_diff?

Codebase docs right now still link to python2, so the type here should be listed as unicode rather than str. …

Please dedent this 4 spaces (align with line above it)

Please add a blank line between these two.

Instead of using the character, put the entire conditional in parens: if (options.color_diff == self.COLOR_ALWAYS or (options.color_diff == self.COLOR_AUTO and …

Standard-library imports should go in their own section just after the __future__ import. Order should be full-packages first, then from …

This can be a lot simpler: self.assertIn(lines_removed_color_pattern, sample_diff_highlighted) self.assertIn(lines_added_color_pattern, sample_diff_highlighted) Might be nice to shorten some of the variable names …

This blank line can go away

six isn't part of the standard library, so this should go in the next import group.

This description could be more accurate.

This should end with a period.

Align the sys on the same column as options above it.

Test -> Tests

Why the .encode() calls? We should be able to operate entirely in unicode-land.

F401 'pygments.highlight' imported but unused

These are both considered third-party modules, so should be in the same import group (no blank line here).

This should also have an added_in='3.0' parameter, to allow the documentation to reflect when this option was first made available.

This should have a trailing period.

I feel this should go somewhere lower than main(). It should also probably be a private function.

Looks like this wraps a bit early. The "or" can easily fit on the previous line.

This can be one statement.

These should ideally use keyword arguments.

DiffLexer() is a good default, but something you can do now that this change is basically complete is support pydiffx's …

This can easily be one statement.

Unit tests should focus on testing one condition at a time. So instead of looping through a set of possible …

Looks like this is wrapping prematurely. More can fit on the first line.

Test docstrings should be in the form of: Testing <thing>[ with <conditions>]. So, Testing rbt diff with --color-diff=....

Small nit: For multi-line strings like this, put the trailing ) on its own line (aligned with the start of …

We try to avoid the multi-line with form. Let's use nested with blocks instead.

As above, Testing <thing>[ with <conditions>]. In this case, <thing> would be Diff.colorize_diff

Unwanted blank line here.

+= is faster than .extends in Python. Let's move to that.

This is more generally useful, and we don't want to maintain a bunch of copies of it. Let's move it …

No reason to create a variable and then return it. Let's just return highlight(...)

Why is the extend() version commented out? That's generally a better choice than +=.