Apply basic colorization to diff

Review Request #11829 — Created Oct. 3, 2021 and updated

Information

RBTools
master

Reviewers

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

  • Created test_diff.py that contains test cases for rbt diff command
  • Tested command arguments and formating the diff using pygments library.
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

Description From Last Updated

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E128 continuation line under-indented for visual indent

reviewbotreviewbot

E501 line too long (94 > 79 characters)

reviewbotreviewbot

E124 closing bracket does not match visual indentation

reviewbotreviewbot

E501 line too long (94 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

E251 unexpected spaces around keyword / parameter equals

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

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

maubinmaubin

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

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

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

daviddavid

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

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E225 missing whitespace around operator

reviewbotreviewbot

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

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

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

reviewbotreviewbot

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

reviewbotreviewbot

W503 line break before binary operator

reviewbotreviewbot

W503 line break before binary operator

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

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

daviddavid

Add a blank line between these two.

daviddavid

This needs to tell us what the type is.

daviddavid

Add a blank line between these two.

daviddavid

Please put this blank line back.

daviddavid

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

daviddavid

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

daviddavid

Please add a blank line between these two

daviddavid

E999 SyntaxError: invalid syntax

reviewbotreviewbot

Please add a blank line between these two

daviddavid

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

daviddavid

Let's use single quotes instead of double here.

daviddavid

This should be (list of unicode, optional)

daviddavid

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

daviddavid

DIFF is incorrectly capitalized here.

daviddavid

Please add a blank line between these two.

daviddavid

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

daviddavid

Please add a blank line between these two.

daviddavid

F401 'pygments.highlight' imported but unused

reviewbotreviewbot

F401 'rbtools.commands.CommandError' imported but unused

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

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

daviddavid

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

daviddavid

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

daviddavid

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

daviddavid

Please add a blank line between these two.

daviddavid

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 …

daviddavid

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

daviddavid

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 …

daviddavid

This blank line can go away

daviddavid

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

daviddavid

This description could be more accurate.

daviddavid

This should end with a period.

daviddavid

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

daviddavid

Test -> Tests

daviddavid

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

daviddavid

F401 'pygments.highlight' imported but unused

reviewbotreviewbot

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

chipx86chipx86

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

chipx86chipx86

This should have a trailing period.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

This can be one statement.

chipx86chipx86

These should ideally use keyword arguments.

chipx86chipx86

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

chipx86chipx86

This can easily be one statement.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Unwanted blank line here.

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

W292 no newline at end of file

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

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

daviddavid

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

daviddavid
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

sandysaji
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
sandysaji
maubin
  1. Overall looks good :)

  2. 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

  3. 
      
sandysaji
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
  1. 
      
  2. 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.

    1. Good idea! I will update the implementation accordingly.

  3. 
      
sandysaji
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
Review request changed
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
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

sandysaji
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
  1. 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?

    1. 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.

  2. rbtools/commands/diff.py (Diff revision 11)
     
     
    Show all issues

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

  3. rbtools/commands/diff.py (Diff revision 11)
     
     
     
    Show all issues

    Add a blank line between these two.

  4. rbtools/commands/diff.py (Diff revision 11)
     
     
    Show all issues

    This needs to tell us what the type is.

  5. rbtools/commands/diff.py (Diff revision 11)
     
     
     
    Show all issues

    Add a blank line between these two.

  6. rbtools/commands/diff.py (Diff revision 11)
     
     
    Show all issues

    Please put this blank line back.

  7. 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?

    1. 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.

  8. 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.

  9. rbtools/commands/tests/test_diff.py (Diff revision 11)
     
     
     
    Show all issues

    Please add a blank line between these two

  10. rbtools/commands/tests/test_diff.py (Diff revision 11)
     
     
     
    Show all issues

    Please add a blank line between these two

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

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

  12. rbtools/commands/tests/test_diff.py (Diff revision 11)
     
     
     
     
     
    Show all issues

    Let's use single quotes instead of double here.

  13. rbtools/commands/tests/test_diff.py (Diff revision 11)
     
     
    Show all issues

    This should be (list of unicode, optional)

  14. 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.

  15. rbtools/commands/tests/test_diff.py (Diff revision 11)
     
     
    Show all issues

    DIFF is incorrectly capitalized here.

  16. rbtools/commands/tests/test_diff.py (Diff revision 11)
     
     
     
    Show all issues

    Please add a blank line between these two.

  17. rbtools/commands/tests/test_diff.py (Diff revision 11)
     
     
    Show all issues

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

  18. rbtools/commands/tests/test_diff.py (Diff revision 11)
     
     
     
    Show all issues

    Please add a blank line between these two.

  19. 
      
sandysaji
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
sandysaji
david
  1. 
      
  2. 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
    ...
    
  3. rbtools/commands/diff.py (Diff revision 13)
     
     
    Show all issues

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

  4. 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.

  5. rbtools/commands/diff.py (Diff revision 13)
     
     
    Show all issues

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

  6. rbtools/commands/diff.py (Diff revision 13)
     
     
     
    Show all issues

    Please add a blank line between these two.

  7. 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())):
    
    1. 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())):
      
  8. 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
    
    1. That makes sense. Thanks!

  9. 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?

    1. Thanks for the suggesstion! It really did simplify it.

  10. rbtools/commands/tests/test_diff.py (Diff revision 13)
     
     
    Show all issues

    This blank line can go away

  11. 
      
sandysaji
david
  1. 
      
  2. 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.

  3. rbtools/commands/diff.py (Diff revision 14)
     
     
    Show all issues

    This description could be more accurate.

  4. rbtools/commands/diff.py (Diff revision 14)
     
     
    Show all issues

    This should end with a period.

  5. rbtools/commands/diff.py (Diff revision 14)
     
     
     
    Show all issues

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

  6. rbtools/commands/tests/test_diff.py (Diff revision 14)
     
     
    Show all issues

    Test -> Tests

  7. 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.

    1. 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'
      
    2. 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)
      
    3. 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

  8. 
      
sandysaji
sandysaji
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
chipx86
  1. 
      
  2. 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).

  3. 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.

  4. rbtools/commands/diff.py (Diff revision 17)
     
     
     
    Show all issues

    This should have a trailing period.

  5. 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.

  6. 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.

  7. rbtools/commands/diff.py (Diff revision 17)
     
     
     
     
     
     
    Show all issues

    This can be one statement.

  8. rbtools/commands/diff.py (Diff revision 17)
     
     
     
     
    Show all issues

    These should ideally use keyword arguments.

  9. 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.

  10. rbtools/commands/diff.py (Diff revision 17)
     
     
     
    Show all issues

    This can easily be one statement.

  11. 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.

  12. 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.

  13. 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=....

  14. 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.

  15. 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.

  16. 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.

  17. 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

  18. rbtools/commands/tests/test_diff.py (Diff revision 17)
     
     
     
    Show all issues

    Unwanted blank line here.

  19. rbtools/commands/tests/test_diff.py (Diff revision 17)
     
     
    Show all issues

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

  20. 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.

  21. 
      
sandysaji
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
david
  1. 
      
  2. 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(...)

  3. 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 +=.

    1. 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

  4. 
      
sandysaji
sandysaji
Review request changed
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