Summary

Apply basic colorization to diff

Review Request #11829 — Created Oct. 3, 2021 and updated Dec. 6, 2021, 5:37 p.m.

Information

Owner

sandysaji

Repository

RBTools

Branch

master

Bugs

Depends On

Reviewers

Groups

rbtools

People

Description

Add an option to the rbt diff command to
apply basic colorization to the output.
The rbt diff --color-diff command highlights
removed lines in red and added lines in green.

This review request includes the following:

Utilizing pygments library, format the diff displayed
on terminal with basic colorization.
Add --color-diff option to rbt diff command.

Project documentation and notes

Testing Done


Created test_diff.py that contains test cases for rbt diff command
Tested command arguments and formating the diff using pygments library.

Commits

Summary	ID
Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
clean up	b77a7aec6879e99921707ca3539374b6175e34d6
Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3
updated function name, sorted import statements and minor code clean ups	0930b630a8dc6f130653be4783c78329f8e144d8
minor code clean ups	02dbed4973f53a17f384482c152216e3ad829046
remove encode method	e03c45a24e8786405780ae4e0d5d0681e15eb4e7
remove unused import	aa1d865cd90045688b73875451bc9bddfba6636b
update colorize_diff to private method, update unit tests to focus on single test cases, moved _capture_sys_output	10f2a81c2141c994505bac6f0eb2d95ccc57121f
Add support and test cases for diffX	8c632cf4a6b77613e4e211b7c6724e0813af40aa

Files

Issues

Description	From	Last Updated
E251 unexpected spaces around keyword / parameter equals	reviewbot	Oct. 3, 2021, 5:25 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	Oct. 3, 2021, 5:25 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Oct. 3, 2021, 5:21 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Oct. 3, 2021, 5:21 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Oct. 3, 2021, 5:21 p.m.
E128 continuation line under-indented for visual indent	reviewbot	Oct. 3, 2021, 5:21 p.m.
E501 line too long (94 > 79 characters)	reviewbot	Oct. 3, 2021, 5:21 p.m.
E124 closing bracket does not match visual indentation	reviewbot	Oct. 3, 2021, 5:21 p.m.
E501 line too long (94 > 79 characters)	reviewbot	Oct. 3, 2021, 5:22 p.m.
E501 line too long (84 > 79 characters)	reviewbot	Oct. 3, 2021, 5:22 p.m.
E225 missing whitespace around operator	reviewbot	Oct. 3, 2021, 5:25 p.m.
W291 trailing whitespace	reviewbot	Oct. 3, 2021, 5:21 p.m.
E501 line too long (84 > 79 characters)	reviewbot	Oct. 3, 2021, 5:21 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	Oct. 3, 2021, 5:24 p.m.
E251 unexpected spaces around keyword / parameter equals	reviewbot	Oct. 3, 2021, 5:24 p.m.
W291 trailing whitespace	reviewbot	Oct. 3, 2021, 5:24 p.m.
W291 trailing whitespace	reviewbot	Oct. 3, 2021, 5:24 p.m.
It might be nicer to do elif six.PY2 and then an else instead of having nested if else statements	maubin	Oct. 12, 2021, 9:17 p.m.
F841 local variable 'diff' is assigned to but never used	reviewbot	Oct. 12, 2021, 9:34 p.m.
E303 too many blank lines (2)	reviewbot	Oct. 12, 2021, 9:34 p.m.
W292 no newline at end of file	reviewbot	Oct. 12, 2021, 9:34 p.m.
Instead of just having this be a boolean, I think we should follow git's lead and have it be a …	david	Nov. 2, 2021, 6:06 p.m.
F841 local variable 'diff' is assigned to but never used	reviewbot	Oct. 20, 2021, 5:30 p.m.
E225 missing whitespace around operator	reviewbot	Oct. 20, 2021, 5:35 p.m.
E501 line too long (80 > 79 characters)	reviewbot	Oct. 20, 2021, 5:35 p.m.
E225 missing whitespace around operator	reviewbot	Oct. 20, 2021, 5:35 p.m.
F841 local variable 'diff' is assigned to but never used	reviewbot	Oct. 20, 2021, 5:35 p.m.
W291 trailing whitespace	reviewbot	Oct. 20, 2021, 5:36 p.m.
F841 local variable 'diff' is assigned to but never used	reviewbot	Oct. 20, 2021, 5:36 p.m.
F841 local variable 'diff' is assigned to but never used	reviewbot	Nov. 2, 2021, 6:04 p.m.
W503 line break before binary operator	reviewbot	Nov. 2, 2021, 6:25 p.m.
W503 line break before binary operator	reviewbot	Nov. 2, 2021, 6:25 p.m.
E999 SyntaxError: invalid syntax	reviewbot	Nov. 2, 2021, 6:25 p.m.
Perhaps we should call this "display options" to distinguish it from Command.diff_options?	david	Nov. 10, 2021, 9:25 a.m.
Add a blank line between these two.	david	Nov. 10, 2021, 9:26 a.m.
This needs to tell us what the type is.	david	Nov. 10, 2021, 5:40 p.m.
Add a blank line between these two.	david	Nov. 10, 2021, 9:26 a.m.
Please put this blank line back.	david	Nov. 10, 2021, 9:27 a.m.
Indentation is a little bit funky here and things don't quite line up with the parens. I think it might …	david	Nov. 16, 2021, 7:30 p.m.
All test docstrings should start with the word "Testing" for consistency in output.	david	Nov. 10, 2021, 9:28 a.m.
Please add a blank line between these two	david	Nov. 10, 2021, 9:29 a.m.
E999 SyntaxError: invalid syntax	reviewbot	Nov. 10, 2021, 5:24 p.m.
Please add a blank line between these two	david	Nov. 10, 2021, 9:38 a.m.
There's an extra space at the end of this docstring.	david	Nov. 10, 2021, 9:38 a.m.
Let's use single quotes instead of double here.	david	Nov. 10, 2021, 9:39 a.m.
This should be (list of unicode, optional)	david	Nov. 10, 2021, 9:39 a.m.
I don't think this adds useful info--the first line of explanation is sufficient.	david	Nov. 10, 2021, 9:40 a.m.
DIFF is incorrectly capitalized here.	david	Nov. 10, 2021, 5:22 p.m.
Please add a blank line between these two.	david	Nov. 10, 2021, 9:40 a.m.
This needs a docstring. It probably should also be prefixed with _	david	Nov. 10, 2021, 9:51 a.m.
Please add a blank line between these two.	david	Nov. 10, 2021, 9:41 a.m.
F401 'pygments.highlight' imported but unused	reviewbot	Nov. 11, 2021, 11:22 a.m.
F401 'rbtools.commands.CommandError' imported but unused	reviewbot	Nov. 11, 2021, 11:22 a.m.
W293 blank line contains whitespace	reviewbot	Nov. 11, 2021, 11:22 a.m.
These should be grouped together with full-package imports coming before from imports. Please also alphabetize them (formatters before lexers). import …	david	Nov. 17, 2021, 10:21 a.m.
Let's give this method an imperative name. Perhaps colorize_diff?	david	Nov. 16, 2021, 7:26 p.m.
Codebase docs right now still link to python2, so the type here should be listed as unicode rather than str. …	david	Nov. 16, 2021, 7:29 p.m.
Please dedent this 4 spaces (align with line above it)	david	Nov. 16, 2021, 7:19 p.m.
Please add a blank line between these two.	david	Nov. 16, 2021, 7:18 p.m.
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 …	david	Nov. 18, 2021, 7:19 p.m.
Standard-library imports should go in their own section just after the __future__ import. Order should be full-packages first, then from …	david	Nov. 17, 2021, 10:10 a.m.
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 …	david	Nov. 16, 2021, 6:19 p.m.
This blank line can go away	david	Nov. 16, 2021, 5:44 p.m.
six isn't part of the standard library, so this should go in the next import group.	david	Nov. 23, 2021, 3:37 p.m.
This description could be more accurate.	david	Nov. 23, 2021, 3:37 p.m.
This should end with a period.	david	Nov. 23, 2021, 3:39 p.m.
Align the sys on the same column as options above it.	david	Nov. 23, 2021, 3:39 p.m.
Test -> Tests	david	Nov. 23, 2021, 3:40 p.m.
Why the .encode() calls? We should be able to operate entirely in unicode-land.	david	Nov. 24, 2021, 4:53 p.m.
F401 'pygments.highlight' imported but unused	reviewbot	Nov. 24, 2021, 4:56 p.m.
These are both considered third-party modules, so should be in the same import group (no blank line here).	chipx86	Nov. 25, 2021, 3:03 p.m.
This should also have an added_in='3.0' parameter, to allow the documentation to reflect when this option was first made available.	chipx86	Nov. 25, 2021, 3:05 p.m.
This should have a trailing period.	chipx86	Nov. 25, 2021, 3:05 p.m.
I feel this should go somewhere lower than main(). It should also probably be a private function.	chipx86	Nov. 27, 2021, 4:40 p.m.
Looks like this wraps a bit early. The "or" can easily fit on the previous line.	chipx86	Nov. 25, 2021, 3:18 p.m.
This can be one statement.	chipx86	Nov. 25, 2021, 3:19 p.m.
These should ideally use keyword arguments.	chipx86	Nov. 25, 2021, 4:40 p.m.
DiffLexer() is a good default, but something you can do now that this change is basically complete is support pydiffx's …	chipx86	Dec. 5, 2021, 10:38 a.m.
This can easily be one statement.	chipx86	Nov. 25, 2021, 4:44 p.m.
Unit tests should focus on testing one condition at a time. So instead of looping through a set of possible …	chipx86	Nov. 27, 2021, 5:10 p.m.
Looks like this is wrapping prematurely. More can fit on the first line.	chipx86	Nov. 25, 2021, 4:46 p.m.
Test docstrings should be in the form of: Testing <thing>[ with <conditions>]. So, Testing rbt diff with --color-diff=....	chipx86	Nov. 27, 2021, 7:41 p.m.
Small nit: For multi-line strings like this, put the trailing ) on its own line (aligned with the start of …	chipx86	Nov. 27, 2021, 7:43 p.m.
It's a small nit, but we use %-formatting instead of .format() in our code.	chipx86	Nov. 27, 2021, 5:21 p.m.
We try to avoid the multi-line with form. Let's use nested with blocks instead.	chipx86	Nov. 27, 2021, 5:24 p.m.
As above, Testing <thing>[ with <conditions>]. In this case, <thing> would be Diff.colorize_diff	chipx86	Nov. 27, 2021, 8:23 p.m.
Unwanted blank line here.	chipx86	Nov. 25, 2021, 4:49 p.m.
+= is faster than .extends in Python. Let's move to that.	chipx86	Nov. 25, 2021, 4:48 p.m.
This is more generally useful, and we don't want to maintain a bunch of copies of it. Let's move it …	chipx86	Nov. 28, 2021, 11:57 a.m.
W292 no newline at end of file	reviewbot	Nov. 28, 2021, 12:12 p.m.
W292 no newline at end of file	reviewbot	Nov. 28, 2021, 12:12 p.m.
No reason to create a variable and then return it. Let's just return highlight(...)	david	Dec. 5, 2021, 10:12 a.m.
Why is the extend() version commented out? That's generally a better choice than +=.	david	Dec. 5, 2021, 10:09 a.m.

flake8 failed.

JSHint passed.

flake8

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

Show all issues

E251 unexpected spaces around keyword / parameter equals

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

Show all issues

E251 unexpected spaces around keyword / parameter equals

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

Show all issues

E128 continuation line under-indented for visual indent

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

Show all issues

E128 continuation line under-indented for visual indent

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

Show all issues

E128 continuation line under-indented for visual indent

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

Show all issues

E128 continuation line under-indented for visual indent

rbtools/commands/diff.py (Diff revision 1)
Show all issues
```
E501 line too long (94 > 79 characters)
```

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

Show all issues

E124 closing bracket does not match visual indentation

rbtools/commands/diff.py (Diff revision 1)
Show all issues
```
E501 line too long (94 > 79 characters)
```
rbtools/commands/diff.py (Diff revision 1)
Show all issues
```
E501 line too long (84 > 79 characters)
```

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c

Diff:

Revision 2 (+66 -20)

Show changes

rbtools/commands/diff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/diff.py (Diff revision 2)
Show all issues
```
E225 missing whitespace around operator
```
rbtools/commands/diff.py (Diff revision 2)
Show all issues
```
W291 trailing whitespace
```
rbtools/commands/diff.py (Diff revision 2)
Show all issues
```
E501 line too long (84 > 79 characters)
```

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6

Diff:

Revision 3 (+74 -22)

Show changes

rbtools/commands/diff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/diff.py (Diff revision 3)

Show all issues

E251 unexpected spaces around keyword / parameter equals

rbtools/commands/diff.py (Diff revision 3)

Show all issues

E251 unexpected spaces around keyword / parameter equals

rbtools/commands/diff.py (Diff revision 3)
Show all issues
```
W291 trailing whitespace
```
rbtools/commands/diff.py (Diff revision 3)
Show all issues
```
W291 trailing whitespace
```

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d

Diff:

Revision 4 (+77 -25)

Show changes

rbtools/commands/diff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Added Files:

```
Overall looks good :)
```
1. sandysaji Oct. 12, 2021, 9:17 p.m.
  Thanks!

rbtools/commands/diff.py (Diff revision 4)

Show all issues

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

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905

Diff:

Revision 5 (+169 -29)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/tests/test_diff.py (Diff revision 5)
Show all issues
```
F841 local variable 'diff' is assigned to but never used
```
rbtools/commands/tests/test_diff.py (Diff revision 5)
Show all issues
```
E303 too many blank lines (2)
```
rbtools/commands/tests/test_diff.py (Diff revision 5)
Show all issues
```
W292 no newline at end of file
```

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349

Diff:

Revision 6 (+169 -33)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/tests/test_diff.py (Diff revision 6)
Show all issues
```
F841 local variable 'diff' is assigned to but never used
```

rbtools/commands/diff.py (Diff revision 6)

Show all issues

Instead of just having this be a boolean, I think we should follow git's lead and have it be a choice between "never", "always", and "auto" (with auto being the default). When using auto, we should check sys.stdout.isatty() to determine whether to use color.

sandysaji Nov. 2, 2021, 6:06 p.m.

Good idea! I will update the implementation accordingly.

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613

Diff:

Revision 7 (+209 -49)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/diff.py (Diff revision 7)
Show all issues
```
E225 missing whitespace around operator
```
rbtools/commands/diff.py (Diff revision 7)
Show all issues
```
E501 line too long (80 > 79 characters)
```
rbtools/commands/diff.py (Diff revision 7)
Show all issues
```
E225 missing whitespace around operator
```
rbtools/commands/tests/test_diff.py (Diff revision 7)
Show all issues
```
F841 local variable 'diff' is assigned to but never used
```

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8

Diff:

Revision 8 (+213 -51)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/diff.py (Diff revision 8)
Show all issues
```
W291 trailing whitespace
```
rbtools/commands/tests/test_diff.py (Diff revision 8)
Show all issues
```
F841 local variable 'diff' is assigned to but never used
```

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba

Diff:

Revision 9 (+214 -52)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/tests/test_diff.py (Diff revision 9)
Show all issues
```
F841 local variable 'diff' is assigned to but never used
```

Change Summary:

New test cases and new screenshots. Minor updates to the diff_colorization function.

Description:

		Add an option to the `rbt diff` command to
		apply basic colorization to the output.
~		The `rbt diff --color` command highlights
	~	The `rbt diff --color-diff` command highlights
		removed lines in red and added lines in green.



		This review request includes the following:

		Utilizing pygments library, format the diff displayed on terminal with basic colorization.
~		Add `--color` option to `rbt diff` command.
	~	Add `--color-diff` option to `rbt diff` command.

Testing Done:

~		Work in progress.
~
	~	Created `test_diff.py` that contains test cases for `rbt diff` command
	~	Tested command arguments and formating the diff using pygments library.
-		Currently, there are not test cases for `rbt diff` command.
-		Therefore, we will need to write test cases for the command.

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113

Diff:

Revision 10 (+340 -74)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py

File Captions:

rbt_diff.png:
- rbt_diff.png
+ rbt_diff (outdated).png
rbt_diff_--color.png:
- rbt_diff_--color.png
+ rbt_diff_--color (outdated).png

Added Files:

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/diff.py (Diff revision 10)
Show all issues
```
W503 line break before binary operator
```
rbtools/commands/diff.py (Diff revision 10)
Show all issues
```
W503 line break before binary operator
```
rbtools/commands/tests/test_diff.py (Diff revision 10)
Show all issues
```
E999 SyntaxError: invalid syntax
```

Change Summary:

New test cases and new screenshots. Minor updates to the diff_colorization function.

Fixed flake8 errors.

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6

Diff:

Revision 11 (+346 -78)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/tests/test_diff.py (Diff revision 11)
Show all issues
```
E999 SyntaxError: invalid syntax
```

This is mostly a bunch of small trivial issues with formatting. I do have one question about string types in the comment on line 100/139.

What is left to do for this change? Should the [WIP] tag be removed?

sandysaji Nov. 11, 2021, 11:30 a.m.

I addressed the small trivial issues with formatting and replied to the string type question in the issue that was opened.
I believe the next step would be to remove [WIP] but I wanted to quickly confirm before I do so.

rbtools/commands/diff.py (Diff revision 11)

Show all issues

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

rbtools/commands/diff.py (Diff revision 11)
Show all issues
```
Add a blank line between these two.
```
rbtools/commands/diff.py (Diff revision 11)
Show all issues
```
This needs to tell us what the type is.
```
rbtools/commands/diff.py (Diff revision 11)
Show all issues
```
Add a blank line between these two.
```
rbtools/commands/diff.py (Diff revision 11)
Show all issues
```
Please put this blank line back.
```

rbtools/commands/diff.py (Diff revision 11)

Show all issues

Indentation is a little bit funky here and things don't quite line up with the parens. I think it might be a little easier to read if we swapped the order:

if (self.options.color_diff == self.COLOR_ALWAYS or
    (self.options.color_diff == self.COLOR_AUTO and
     sys.stdout.isatty())):


Note how we align with the parens for both cases.
I also have a question here: On Python 2, we can be pretty freewheeling with string types, but things are stricter on Python 3. Does Pygments return str or bytes?

sandysaji Nov. 11, 2021, 11:30 a.m.

I have updated the if statement by swapping the order, fixing the alignment and making it more easier to read.
Let me know if it this is better or if we can further improve it.

Pygments returns str.

rbtools/commands/tests/test_diff.py (Diff revision 11)

Show all issues

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

rbtools/commands/tests/test_diff.py (Diff revision 11)
Show all issues
```
Please add a blank line between these two
```
rbtools/commands/tests/test_diff.py (Diff revision 11)
Show all issues
```
Please add a blank line between these two
```
rbtools/commands/tests/test_diff.py (Diff revision 11)
Show all issues
```
There's an extra space at the end of this docstring.
```
rbtools/commands/tests/test_diff.py (Diff revision 11)
Show all issues
```
Let's use single quotes instead of double here.
```
rbtools/commands/tests/test_diff.py (Diff revision 11)
Show all issues
```
This should be (list of unicode, optional)
```

rbtools/commands/tests/test_diff.py (Diff revision 11)

Show all issues

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

rbtools/commands/tests/test_diff.py (Diff revision 11)
Show all issues
```
DIFF is incorrectly capitalized here.
```
rbtools/commands/tests/test_diff.py (Diff revision 11)
Show all issues
```
Please add a blank line between these two.
```

rbtools/commands/tests/test_diff.py (Diff revision 11)

Show all issues

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

rbtools/commands/tests/test_diff.py (Diff revision 11)
Show all issues
```
Please add a blank line between these two.
```

Change Summary:

Minor updates to code which addresses feedbacks provided on the review request.

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b

Diff:

Revision 12 (+380 -96)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/tests/test_diff.py (Diff revision 12)
Show all issues
```
F401 'pygments.highlight' imported but unused
```
rbtools/commands/tests/test_diff.py (Diff revision 12)
Show all issues
```
F401 'rbtools.commands.CommandError' imported but unused
```
rbtools/commands/tests/test_diff.py (Diff revision 12)
Show all issues
```
W293 blank line contains whitespace
```

Change Summary:

Fixed issues rasied by Review Bot. Minor updates to code which addresses feedbacks provided on the review request.

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3

Diff:

Revision 13 (+378 -100)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Summary:

-	[WIP]: Apply basic colorization to diff
+	Apply basic colorization to diff

rbtools/commands/diff.py (Diff revision 13)

Show all issues

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

import six
from pygments import highlight
...

rbtools/commands/diff.py (Diff revision 13)

Show all issues

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

rbtools/commands/diff.py (Diff revision 13)

Show all issues

Codebase docs right now still link to python2, so the type here should be listed as unicode rather than str. This line should also just be a type, not list any kind of variable name.

rbtools/commands/diff.py (Diff revision 13)

Show all issues

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

rbtools/commands/diff.py (Diff revision 13)
Show all issues
```
Please add a blank line between these two.
```

rbtools/commands/diff.py (Diff revision 13)

Show all issues

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 sys.stdout.isatty())):

sandysaji Nov. 17, 2021, 10:26 a.m.

Sounds good. I wanted to fit the second condition in a single line, to make it easier to read, so I used the \ character instead of the parens as a workaround for the line too long (80 > 79 characters) error. I have updated the conditional statement as seen below to avoid the line too long (80 > 79 characters) error resulting from the extra parens.

if (options.color_diff == self.COLOR_ALWAYS or
    (options.color_diff == self.COLOR_AUTO and
        sys.stdout.isatty())):

rbtools/commands/tests/test_diff.py (Diff revision 13)

Show all issues

Standard-library imports should go in their own section just after the __future__ import. Order should be full-packages first, then from imports, and everything alphabetized. This also needs to use six in order to get the right StringIO implementation on both python 2 and 3 (which goes in its own import group as a "third party module"):

from __future__ import unicode_literals

import sys
from contextlib import contextmanager

from six.moves import cStringIO as StringIO

from rbtools.commands.diff import Diff
from rbtools.utils.testbase import RBTestBase

sandysaji Nov. 17, 2021, 10:26 a.m.
```
That makes sense. Thanks!
```

rbtools/commands/tests/test_diff.py (Diff revision 13)

Show all issues

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 too. Perhaps just highlighted, added_color, and removed_color?

sandysaji Nov. 17, 2021, 10:26 a.m.

Thanks for the suggesstion! It really did simplify it.

rbtools/commands/tests/test_diff.py (Diff revision 13)
Show all issues
```
This blank line can go away
```

Change Summary:

Ordered import statments, updated function name and test cases, and addressed feedback on review request.

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3
	updated function name, sorted import statements and minor code clean ups	0930b630a8dc6f130653be4783c78329f8e144d8

Diff:

Revision 14 (+400 -124)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/commands/diff.py (Diff revision 14)

Show all issues

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

rbtools/commands/diff.py (Diff revision 14)
Show all issues
```
This description could be more accurate.
```
rbtools/commands/diff.py (Diff revision 14)
Show all issues
```
This should end with a period.
```

rbtools/commands/diff.py (Diff revision 14)

Show all issues

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

rbtools/commands/tests/test_diff.py (Diff revision 14)
Show all issues
```
Test -> Tests
```

rbtools/commands/tests/test_diff.py (Diff revision 14)

Show all issues

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

sandysaji Nov. 23, 2021, 4:42 p.m.

Short answer: We are only using .encode() for testing purposes because the highlight method from pyments returns a str.

I was having difficulty coming up with a test case to test this specific feature. As Christian and Mike pointed out during one of the meetings, we simply need to test if the diff is colourized rather than checking the lexers and formatters from Pygments individually. Reading through the documentation, I discovered that the colour of the string is determined by escape sequences, such as x1b[91m, x1b[32m. Therefore, I used .encode() to check for the presence of specific escape sequences to ensure that the strings were coloured.

sample_diff: b'diff --git a/rbtools/commands/diff.py b/rbtools/commands/diff.py\n--- a/rbtools/commands/diff.py\n+++ b/rbtools/commands/diff.py\n'

highlighted.encode(): b’\x1b[01mdiff --git a/rbtools/commands/diff.py b/rbtools/commands/diff.py\x1b[39;49;00m\n\x1b[91m--- a/rbtools/commands/diff.py\x1b[39;49;00m\n\x1b[32m+++ b/rbtools/commands/diff.py\x1b[39;49;00m\n'

david Nov. 23, 2021, 11:05 p.m.

My point is, we're starting with removed_color, added_color, and highlighted all as str. Calling .encode() converts them all to bytes. But the in checks should work fine for strs. So why can't we just do:

self.assertIn(removed_color, highlighted)
self.assertIn(added_color, highlighted)

sandysaji Nov. 24, 2021, 4:52 p.m.

Sorry about that, I misread what you were saying. Yes, you're right. We don't need to use .encode() and it can work just using str. I've modified the code to reflect the new modifications and will close this ticket. Thanks

Change Summary:

Minor code changes to address feedback on review request.

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3
	updated function name, sorted import statements and minor code clean ups	0930b630a8dc6f130653be4783c78329f8e144d8
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3
	updated function name, sorted import statements and minor code clean ups	0930b630a8dc6f130653be4783c78329f8e144d8
	minor code clean ups	02dbed4973f53a17f384482c152216e3ad829046

Diff:

Revision 15 (+406 -128)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Removed .encode() and updated self.assertIn.

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3
	updated function name, sorted import statements and minor code clean ups	0930b630a8dc6f130653be4783c78329f8e144d8
	minor code clean ups	02dbed4973f53a17f384482c152216e3ad829046
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3
	updated function name, sorted import statements and minor code clean ups	0930b630a8dc6f130653be4783c78329f8e144d8
	minor code clean ups	02dbed4973f53a17f384482c152216e3ad829046
	remove encode method	e03c45a24e8786405780ae4e0d5d0681e15eb4e7

Diff:

Revision 16 (+410 -130)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/tests/test_diff.py (Diff revision 16)
Show all issues
```
F401 'pygments.highlight' imported but unused
```

Change Summary:

Removed unused imports.

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3
	updated function name, sorted import statements and minor code clean ups	0930b630a8dc6f130653be4783c78329f8e144d8
	minor code clean ups	02dbed4973f53a17f384482c152216e3ad829046
	remove encode method	e03c45a24e8786405780ae4e0d5d0681e15eb4e7
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3
	updated function name, sorted import statements and minor code clean ups	0930b630a8dc6f130653be4783c78329f8e144d8
	minor code clean ups	02dbed4973f53a17f384482c152216e3ad829046
	remove encode method	e03c45a24e8786405780ae4e0d5d0681e15eb4e7
	remove unused import	aa1d865cd90045688b73875451bc9bddfba6636b

Diff:

Revision 17 (+409 -131)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/commands/diff.py (Diff revision 17)

Show all issues

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

rbtools/commands/diff.py (Diff revision 17)

Show all issues

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

rbtools/commands/diff.py (Diff revision 17)
Show all issues
```
This should have a trailing period.
```

rbtools/commands/diff.py (Diff revision 17)

Show all issues

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

rbtools/commands/diff.py (Diff revision 17)

Show all issues

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

rbtools/commands/diff.py (Diff revision 17)
Show all issues
```
This can be one statement.
```

rbtools/commands/diff.py (Diff revision 17)

Show all issues

These should ideally use keyword arguments.

rbtools/commands/diff.py (Diff revision 17)

Show all issues

DiffLexer() is a good default, but something you can do now that this change is basically complete is support pydiffx's pydiffx.integrations.pygments_lexer.DiffXLexer when diff.startswith(b'#diffx').

Unit tests would be necessary to make sure that's handled right.

rbtools/commands/diff.py (Diff revision 17)
Show all issues
```
This can easily be one statement.
```

rbtools/commands/tests/test_diff.py (Diff revision 17)

Show all issues

Unit tests should focus on testing one condition at a time. So instead of looping through a set of possible options and checking results, you should have a unit test for each possible option, and document that in the docstring. When there's a regression, it becomes easier to spot the extent of the fallout.
You can have a private convenience function that does the bulk of the test and call it from each conditional test function.

rbtools/commands/tests/test_diff.py (Diff revision 17)

Show all issues

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

rbtools/commands/tests/test_diff.py (Diff revision 17)

Show all issues

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

rbtools/commands/tests/test_diff.py (Diff revision 17)

Show all issues

Small nit: For multi-line strings like this, put the trailing ) on its own line (aligned with the start of the variable name defining the string).

You'll do this below as well.

rbtools/commands/tests/test_diff.py (Diff revision 17)

Show all issues

It's a small nit, but we use %-formatting instead of .format() in our code.

rbtools/commands/tests/test_diff.py (Diff revision 17)

Show all issues

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

rbtools/commands/tests/test_diff.py (Diff revision 17)

Show all issues

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

rbtools/commands/tests/test_diff.py (Diff revision 17)
Show all issues
```
Unwanted blank line here.
```

rbtools/commands/tests/test_diff.py (Diff revision 17)

Show all issues

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

rbtools/commands/tests/test_diff.py (Diff revision 17)

Show all issues

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

Change Summary:

updated colorize_diff to private method, unit tests to focus on single test cases, and _capture_sys_output. Addressed feedbacks on review request.

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3
	updated function name, sorted import statements and minor code clean ups	0930b630a8dc6f130653be4783c78329f8e144d8
	minor code clean ups	02dbed4973f53a17f384482c152216e3ad829046
	remove encode method	e03c45a24e8786405780ae4e0d5d0681e15eb4e7
	remove unused import	aa1d865cd90045688b73875451bc9bddfba6636b
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3
	updated function name, sorted import statements and minor code clean ups	0930b630a8dc6f130653be4783c78329f8e144d8
	minor code clean ups	02dbed4973f53a17f384482c152216e3ad829046
	remove encode method	e03c45a24e8786405780ae4e0d5d0681e15eb4e7
	remove unused import	aa1d865cd90045688b73875451bc9bddfba6636b
	update colorize_diff to private method, update unit tests to focus on single test cases, moved _capture_sys_output	1f0b992978ce8462d3407c57591eccad1b1da4e8

Diff:

Revision 18 (+464 -196)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py
	rbtools/testing/testcase.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

rbtools/commands/diff.py (Diff revision 18)
Show all issues
```
W292 no newline at end of file
```
rbtools/commands/tests/test_diff.py (Diff revision 18)
Show all issues
```
W292 no newline at end of file
```

Change Summary:

Updated colorize_diff to private method, unit tests to focus on single test cases, and _capture_sys_output. Addressed feedbacks on review request.

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3
	updated function name, sorted import statements and minor code clean ups	0930b630a8dc6f130653be4783c78329f8e144d8
	minor code clean ups	02dbed4973f53a17f384482c152216e3ad829046
	remove encode method	e03c45a24e8786405780ae4e0d5d0681e15eb4e7
	remove unused import	aa1d865cd90045688b73875451bc9bddfba6636b
	update colorize_diff to private method, update unit tests to focus on single test cases, moved _capture_sys_output	1f0b992978ce8462d3407c57591eccad1b1da4e8
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3
	updated function name, sorted import statements and minor code clean ups	0930b630a8dc6f130653be4783c78329f8e144d8
	minor code clean ups	02dbed4973f53a17f384482c152216e3ad829046
	remove encode method	e03c45a24e8786405780ae4e0d5d0681e15eb4e7
	remove unused import	aa1d865cd90045688b73875451bc9bddfba6636b
	update colorize_diff to private method, update unit tests to focus on single test cases, moved _capture_sys_output	10f2a81c2141c994505bac6f0eb2d95ccc57121f

Diff:

Revision 19 (+463 -195)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py
	rbtools/testing/testcase.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

rbtools/commands/diff.py (Diff revision 19)

Show all issues

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

rbtools/commands/tests/test_diff.py (Diff revision 19)

Show all issues

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

sandysaji Dec. 1, 2021, 6:19 p.m.

Oh I missed that. I was intending to remove extend(). Christian mentioned in this issue to replace extend() with += as it is faster in python

Change Summary:

Added support for pydiffx and created test cases. Addressed issues logged.

Commits:

	Summary	ID
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3
	updated function name, sorted import statements and minor code clean ups	0930b630a8dc6f130653be4783c78329f8e144d8
	minor code clean ups	02dbed4973f53a17f384482c152216e3ad829046
	remove encode method	e03c45a24e8786405780ae4e0d5d0681e15eb4e7
	remove unused import	aa1d865cd90045688b73875451bc9bddfba6636b
	update colorize_diff to private method, update unit tests to focus on single test cases, moved _capture_sys_output	10f2a81c2141c994505bac6f0eb2d95ccc57121f
	Apply basic colorization to diff	622bd97c692a5d19caae495823376b8cd233c6c5
	clean up	b77a7aec6879e99921707ca3539374b6175e34d6
	Update code to meet PEP8 coding standards	c0515acf1111028bfc17ee798e843acf18ded18c
	Update code to meet PEP8 coding standards	5bcb68b609c9a7eb1dce1c6c1a9015579fb117d6
	Update code to meet PEP8 coding standards	a787bc38006b09645758ea678caa37d80ce3e35d
	Updated if-else statement with elif	9ac91d774f655263e4ac081b24880a3450cb835e
	clean up file	2ebb07e3c1b77b3e84adb80a28375658bb517d49
	Inital test for rbt diff command	cc1d683c47620bf3f460096270db103d26300905
	fix flake8 error and minor update to test file	b4387fbb996be884259c7615a411ac04e16bb349
	Update color option to utilize choices instead of boolean	b6e2af6fa0633639e3c68e431f4de3416218df00
	remove print statements	318d2b98f4f5e5a9f3e89e40a1526f5ba310e613
	fix flake8 errors	a16745ed2856170647bcfc0a5d5e9e159cc5b5e8
	fix flake8 errors	87ad76ef3a2c938a42493ab1003857e4b52e92ba
	update diff color function to return highlighted diff and clean up file	2f3d0bac30dedb4c74b199f36d5ab1c8f4e8888f
	add test cases and clean up file	c4d389ccc9d5a052011649d5972a9d9c7cd72c67
	clean up code to meeting coding standards	8c35cf1a6eb99524fa3f9eac2dadd14d5ccce6da
	clean up test file to meeting coding standards	af1ca798e79c3a559fc8c6c06e015aa8d07fd113
	fix flake8 errors	798b06c140af403c0e280ebcdcc0f2ac7fccf5f6
	documented code and addressd feedbacks	00eb30d61779430441ea66adc7b591c96ff8ee2b
	fix flake8 errors	156975bef5c64c27d5a4df207a3b7fa98ce933a3
	updated function name, sorted import statements and minor code clean ups	0930b630a8dc6f130653be4783c78329f8e144d8
	minor code clean ups	02dbed4973f53a17f384482c152216e3ad829046
	remove encode method	e03c45a24e8786405780ae4e0d5d0681e15eb4e7
	remove unused import	aa1d865cd90045688b73875451bc9bddfba6636b
	update colorize_diff to private method, update unit tests to focus on single test cases, moved _capture_sys_output	10f2a81c2141c994505bac6f0eb2d95ccc57121f
	Add support and test cases for diffX	8c632cf4a6b77613e4e211b7c6724e0813af40aa

Diff:

Revision 20 (+566 -204)

Show changes

	rbtools/commands/diff.py
	rbtools/commands/tests/test_diff.py
	rbtools/testing/testcase.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Description:

		Add an option to the `rbt diff` command to
		apply basic colorization to the output.
		The `rbt diff --color-diff` command highlights
		removed lines in red and added lines in green.



		This review request includes the following:

		Utilizing pygments library, format the diff displayed on terminal with basic colorization.
		Add `--color-diff` option to `rbt diff` command.
	+
	+	Project documentation and notes

Apply basic colorization to diff

Information

Reviewers

flake8

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (2 succeeded)

Added Files:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Description:

Testing Done:

Commits:

Diff:

File Captions:

Added Files:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Commits:

Diff:

Checks run (2 succeeded)

Summary:

Change Summary:

Commits:

Diff:

Checks run (2 succeeded)

Change Summary:

Commits:

Diff:

Checks run (2 succeeded)

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Commits:

Diff:

Checks run (2 succeeded)

Change Summary:

Commits:

Diff: