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
Apply basic colorization to diff
clean up
Update code to meet PEP8 coding standards
Update code to meet PEP8 coding standards
Update code to meet PEP8 coding standards
Updated if-else statement with elif
clean up file
Inital test for rbt diff command
fix flake8 error and minor update to test file
Update color option to utilize choices instead of boolean
remove print statements
fix flake8 errors
fix flake8 errors
update diff color function to return highlighted diff and clean up file
add test cases and clean up file
clean up code to meeting coding standards
clean up test file to meeting coding standards
fix flake8 errors
documented code and addressd feedbacks
fix flake8 errors
updated function name, sorted import statements and minor code clean ups
minor code clean ups
remove encode method
remove unused import
update colorize_diff to private method, update unit tests to focus on single test cases, moved _capture_sys_output
Add support and test cases for diffX

Screenshots

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
-	Apply basic colorization to diff
-	clean up
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Updated if-else statement with elif
-	clean up file
-	Inital test for rbt diff command
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command
+	fix flake8 error and minor update to test file

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Updated if-else statement with elif
-	clean up file
-	Inital test for rbt diff command
-	fix flake8 error and minor update to test file
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command
+	fix flake8 error and minor update to test file
+	Update color option to utilize choices instead of boolean
+	remove print statements

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Updated if-else statement with elif
-	clean up file
-	Inital test for rbt diff command
-	fix flake8 error and minor update to test file
-	Update color option to utilize choices instead of boolean
-	remove print statements
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command
+	fix flake8 error and minor update to test file
+	Update color option to utilize choices instead of boolean
+	remove print statements
+	fix flake8 errors

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Updated if-else statement with elif
-	clean up file
-	Inital test for rbt diff command
-	fix flake8 error and minor update to test file
-	Update color option to utilize choices instead of boolean
-	remove print statements
-	fix flake8 errors
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command
+	fix flake8 error and minor update to test file
+	Update color option to utilize choices instead of boolean
+	remove print statements
+	fix flake8 errors
+	fix flake8 errors

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Updated if-else statement with elif
-	clean up file
-	Inital test for rbt diff command
-	fix flake8 error and minor update to test file
-	Update color option to utilize choices instead of boolean
-	remove print statements
-	fix flake8 errors
-	fix flake8 errors
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command
+	fix flake8 error and minor update to test file
+	Update color option to utilize choices instead of boolean
+	remove print statements
+	fix flake8 errors
+	fix flake8 errors
+	update diff color function to return highlighted diff and clean up file
+	add test cases and clean up file
+	clean up code to meeting coding standards
+	clean up test file to meeting coding standards

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Updated if-else statement with elif
-	clean up file
-	Inital test for rbt diff command
-	fix flake8 error and minor update to test file
-	Update color option to utilize choices instead of boolean
-	remove print statements
-	fix flake8 errors
-	fix flake8 errors
-	update diff color function to return highlighted diff and clean up file
-	add test cases and clean up file
-	clean up code to meeting coding standards
-	clean up test file to meeting coding standards
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command
+	fix flake8 error and minor update to test file
+	Update color option to utilize choices instead of boolean
+	remove print statements
+	fix flake8 errors
+	fix flake8 errors
+	update diff color function to return highlighted diff and clean up file
+	add test cases and clean up file
+	clean up code to meeting coding standards
+	clean up test file to meeting coding standards
+	fix flake8 errors

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Updated if-else statement with elif
-	clean up file
-	Inital test for rbt diff command
-	fix flake8 error and minor update to test file
-	Update color option to utilize choices instead of boolean
-	remove print statements
-	fix flake8 errors
-	fix flake8 errors
-	update diff color function to return highlighted diff and clean up file
-	add test cases and clean up file
-	clean up code to meeting coding standards
-	clean up test file to meeting coding standards
-	fix flake8 errors
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command
+	fix flake8 error and minor update to test file
+	Update color option to utilize choices instead of boolean
+	remove print statements
+	fix flake8 errors
+	fix flake8 errors
+	update diff color function to return highlighted diff and clean up file
+	add test cases and clean up file
+	clean up code to meeting coding standards
+	clean up test file to meeting coding standards
+	fix flake8 errors
+	documented code and addressd feedbacks

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Updated if-else statement with elif
-	clean up file
-	Inital test for rbt diff command
-	fix flake8 error and minor update to test file
-	Update color option to utilize choices instead of boolean
-	remove print statements
-	fix flake8 errors
-	fix flake8 errors
-	update diff color function to return highlighted diff and clean up file
-	add test cases and clean up file
-	clean up code to meeting coding standards
-	clean up test file to meeting coding standards
-	fix flake8 errors
-	documented code and addressd feedbacks
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command
+	fix flake8 error and minor update to test file
+	Update color option to utilize choices instead of boolean
+	remove print statements
+	fix flake8 errors
+	fix flake8 errors
+	update diff color function to return highlighted diff and clean up file
+	add test cases and clean up file
+	clean up code to meeting coding standards
+	clean up test file to meeting coding standards
+	fix flake8 errors
+	documented code and addressd feedbacks
+	fix flake8 errors

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Updated if-else statement with elif
-	clean up file
-	Inital test for rbt diff command
-	fix flake8 error and minor update to test file
-	Update color option to utilize choices instead of boolean
-	remove print statements
-	fix flake8 errors
-	fix flake8 errors
-	update diff color function to return highlighted diff and clean up file
-	add test cases and clean up file
-	clean up code to meeting coding standards
-	clean up test file to meeting coding standards
-	fix flake8 errors
-	documented code and addressd feedbacks
-	fix flake8 errors
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command
+	fix flake8 error and minor update to test file
+	Update color option to utilize choices instead of boolean
+	remove print statements
+	fix flake8 errors
+	fix flake8 errors
+	update diff color function to return highlighted diff and clean up file
+	add test cases and clean up file
+	clean up code to meeting coding standards
+	clean up test file to meeting coding standards
+	fix flake8 errors
+	documented code and addressd feedbacks
+	fix flake8 errors
+	updated function name, sorted import statements and minor code clean ups

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Updated if-else statement with elif
-	clean up file
-	Inital test for rbt diff command
-	fix flake8 error and minor update to test file
-	Update color option to utilize choices instead of boolean
-	remove print statements
-	fix flake8 errors
-	fix flake8 errors
-	update diff color function to return highlighted diff and clean up file
-	add test cases and clean up file
-	clean up code to meeting coding standards
-	clean up test file to meeting coding standards
-	fix flake8 errors
-	documented code and addressd feedbacks
-	fix flake8 errors
-	updated function name, sorted import statements and minor code clean ups
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command
+	fix flake8 error and minor update to test file
+	Update color option to utilize choices instead of boolean
+	remove print statements
+	fix flake8 errors
+	fix flake8 errors
+	update diff color function to return highlighted diff and clean up file
+	add test cases and clean up file
+	clean up code to meeting coding standards
+	clean up test file to meeting coding standards
+	fix flake8 errors
+	documented code and addressd feedbacks
+	fix flake8 errors
+	updated function name, sorted import statements and minor code clean ups
+	minor code clean ups

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Updated if-else statement with elif
-	clean up file
-	Inital test for rbt diff command
-	fix flake8 error and minor update to test file
-	Update color option to utilize choices instead of boolean
-	remove print statements
-	fix flake8 errors
-	fix flake8 errors
-	update diff color function to return highlighted diff and clean up file
-	add test cases and clean up file
-	clean up code to meeting coding standards
-	clean up test file to meeting coding standards
-	fix flake8 errors
-	documented code and addressd feedbacks
-	fix flake8 errors
-	updated function name, sorted import statements and minor code clean ups
-	minor code clean ups
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command
+	fix flake8 error and minor update to test file
+	Update color option to utilize choices instead of boolean
+	remove print statements
+	fix flake8 errors
+	fix flake8 errors
+	update diff color function to return highlighted diff and clean up file
+	add test cases and clean up file
+	clean up code to meeting coding standards
+	clean up test file to meeting coding standards
+	fix flake8 errors
+	documented code and addressd feedbacks
+	fix flake8 errors
+	updated function name, sorted import statements and minor code clean ups
+	minor code clean ups
+	remove encode method

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Updated if-else statement with elif
-	clean up file
-	Inital test for rbt diff command
-	fix flake8 error and minor update to test file
-	Update color option to utilize choices instead of boolean
-	remove print statements
-	fix flake8 errors
-	fix flake8 errors
-	update diff color function to return highlighted diff and clean up file
-	add test cases and clean up file
-	clean up code to meeting coding standards
-	clean up test file to meeting coding standards
-	fix flake8 errors
-	documented code and addressd feedbacks
-	fix flake8 errors
-	updated function name, sorted import statements and minor code clean ups
-	minor code clean ups
-	remove encode method
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command
+	fix flake8 error and minor update to test file
+	Update color option to utilize choices instead of boolean
+	remove print statements
+	fix flake8 errors
+	fix flake8 errors
+	update diff color function to return highlighted diff and clean up file
+	add test cases and clean up file
+	clean up code to meeting coding standards
+	clean up test file to meeting coding standards
+	fix flake8 errors
+	documented code and addressd feedbacks
+	fix flake8 errors
+	updated function name, sorted import statements and minor code clean ups
+	minor code clean ups
+	remove encode method
+	remove unused import

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Updated if-else statement with elif
-	clean up file
-	Inital test for rbt diff command
-	fix flake8 error and minor update to test file
-	Update color option to utilize choices instead of boolean
-	remove print statements
-	fix flake8 errors
-	fix flake8 errors
-	update diff color function to return highlighted diff and clean up file
-	add test cases and clean up file
-	clean up code to meeting coding standards
-	clean up test file to meeting coding standards
-	fix flake8 errors
-	documented code and addressd feedbacks
-	fix flake8 errors
-	updated function name, sorted import statements and minor code clean ups
-	minor code clean ups
-	remove encode method
-	remove unused import
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command
+	fix flake8 error and minor update to test file
+	Update color option to utilize choices instead of boolean
+	remove print statements
+	fix flake8 errors
+	fix flake8 errors
+	update diff color function to return highlighted diff and clean up file
+	add test cases and clean up file
+	clean up code to meeting coding standards
+	clean up test file to meeting coding standards
+	fix flake8 errors
+	documented code and addressd feedbacks
+	fix flake8 errors
+	updated function name, sorted import statements and minor code clean ups
+	minor code clean ups
+	remove encode method
+	remove unused import
+	update colorize_diff to private method, update unit tests to focus on single test cases, moved _capture_sys_output

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Updated if-else statement with elif
-	clean up file
-	Inital test for rbt diff command
-	fix flake8 error and minor update to test file
-	Update color option to utilize choices instead of boolean
-	remove print statements
-	fix flake8 errors
-	fix flake8 errors
-	update diff color function to return highlighted diff and clean up file
-	add test cases and clean up file
-	clean up code to meeting coding standards
-	clean up test file to meeting coding standards
-	fix flake8 errors
-	documented code and addressd feedbacks
-	fix flake8 errors
-	updated function name, sorted import statements and minor code clean ups
-	minor code clean ups
-	remove encode method
-	remove unused import
-	update colorize_diff to private method, update unit tests to focus on single test cases, moved _capture_sys_output
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command
+	fix flake8 error and minor update to test file
+	Update color option to utilize choices instead of boolean
+	remove print statements
+	fix flake8 errors
+	fix flake8 errors
+	update diff color function to return highlighted diff and clean up file
+	add test cases and clean up file
+	clean up code to meeting coding standards
+	clean up test file to meeting coding standards
+	fix flake8 errors
+	documented code and addressd feedbacks
+	fix flake8 errors
+	updated function name, sorted import statements and minor code clean ups
+	minor code clean ups
+	remove encode method
+	remove unused import
+	update colorize_diff to private method, update unit tests to focus on single test cases, moved _capture_sys_output

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
-	Apply basic colorization to diff
-	clean up
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Update code to meet PEP8 coding standards
-	Updated if-else statement with elif
-	clean up file
-	Inital test for rbt diff command
-	fix flake8 error and minor update to test file
-	Update color option to utilize choices instead of boolean
-	remove print statements
-	fix flake8 errors
-	fix flake8 errors
-	update diff color function to return highlighted diff and clean up file
-	add test cases and clean up file
-	clean up code to meeting coding standards
-	clean up test file to meeting coding standards
-	fix flake8 errors
-	documented code and addressd feedbacks
-	fix flake8 errors
-	updated function name, sorted import statements and minor code clean ups
-	minor code clean ups
-	remove encode method
-	remove unused import
-	update colorize_diff to private method, update unit tests to focus on single test cases, moved _capture_sys_output
+	Apply basic colorization to diff
+	clean up
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Update code to meet PEP8 coding standards
+	Updated if-else statement with elif
+	clean up file
+	Inital test for rbt diff command
+	fix flake8 error and minor update to test file
+	Update color option to utilize choices instead of boolean
+	remove print statements
+	fix flake8 errors
+	fix flake8 errors
+	update diff color function to return highlighted diff and clean up file
+	add test cases and clean up file
+	clean up code to meeting coding standards
+	clean up test file to meeting coding standards
+	fix flake8 errors
+	documented code and addressd feedbacks
+	fix flake8 errors
+	updated function name, sorted import statements and minor code clean ups
+	minor code clean ups
+	remove encode method
+	remove unused import
+	update colorize_diff to private method, update unit tests to focus on single test cases, moved _capture_sys_output
+	Add support and test cases for diffX

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

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:

Checks run (1 failed, 1 succeeded)

flake8