Apply basic colorization to diff

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

sandysaji
RBTools
master
rbtools

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

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
Review request changed

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

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)
     
     

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
  1. 
      
  2. rbtools/commands/diff.py (Diff revision 6)
     
     
     

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
Review request changed

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

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

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

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)
     
     

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

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

    Add a blank line between these two.

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

    This needs to tell us what the type is.

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

    Add a blank line between these two.

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

    Please put this blank line back.

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

    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)
     
     

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

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

    Please add a blank line between these two

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

    Please add a blank line between these two

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

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

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

    Let's use single quotes instead of double here.

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

    This should be (list of unicode, optional)

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

    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)
     
     

    DIFF is incorrectly capitalized here.

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

    Please add a blank line between these two.

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

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

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

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
sandysaji
david
  1. 
      
  2. rbtools/commands/diff.py (Diff revision 13)
     
     
     
     
     
     

    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)
     
     

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

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

    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)
     
     

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

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

    Please add a blank line between these two.

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

    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)
     
     
     
     

    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)
     
     
     
     
     

    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)
     
     

    This blank line can go away

  11. 
      
sandysaji
david
  1. 
      
  2. rbtools/commands/diff.py (Diff revision 14)
     
     

    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)
     
     

    This description could be more accurate.

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

    This should end with a period.

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

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

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

    Test -> Tests

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

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
chipx86
  1. 
      
  2. rbtools/commands/diff.py (Diff revision 17)
     
     
     
     

    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)
     
     

    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)
     
     
     

    This should have a trailing period.

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

    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)
     
     
     

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

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

    This can be one statement.

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

    These should ideally use keyword arguments.

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

    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)
     
     
     

    This can easily be one statement.

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

    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)
     
     
     

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

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

    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)
     
     

    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)
     
     

    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)
     
     
     

    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)
     
     

    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)
     
     
     

    Unwanted blank line here.

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

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

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

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

sandysaji
david
  1. 
      
  2. rbtools/commands/diff.py (Diff revision 19)
     
     
     
     

    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)
     
     
     

    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

Loading...