[WIP] Add a setting for mapping file extensions to syntax highlighters for diffs

Review Request #11821 — Created Sept. 24, 2021 and updated

maubin
Review Board
master
reviewboard
Adds a default mapping of file extensions to pygments lexers, which
administrators may modify to define their own custom mapping. If a
mapping is not defined for a certain file extension, we revert to
the old behaviour of guessing which lexer to use given the filename
and file contents.

Created the following unit tests in reviewboard.diffviewer.tests.test_raw_diff_chunk_generator:
- test_apply_pygments_with_custom_mapping() which tests if custom lexer mapping works and if it overrides the old behaviour of guessing
- test_apply_pygments_with_bad_custom_mapping() which tests if proper error is logged when the custom mapping maps an extension to a non existant lexer class

Ran all unit tests in reviewboard.diffviewer with success

Summary Author
Add a setting for mapping file extensions to syntax highlighters for
Michelle
Uses for else clause and .items() instead of .keys() for searching the
Michelle
Logs an error if a mapped lexer does not exist in Pygments
Michelle
Removed trailing whitespace
Michelle
Properly imports mapping from settings.py
Michelle
fixed string formatting for logged error
Michelle
Made CUSTOM_PYGMENTS_LEXERS a class attribute (this makes it easier
Michelle
took out print statement from testing
Michelle
Added unit tests for apply_pygment, testing that default
Michelle
Modified doc formatting and changed default test to instead
Michelle
fixed flake8 issues
Michelle
fixed one last flake8 indent issue
Michelle
better logging message, fixed formatting issues, removed mapping attribute and left it as constant from settings
Michelle
fixed a couple flake8 issues
Michelle
moved mapping to siteconfig instead of settings.py
Michelle
initial form for mapping in diff viewer settings
Michelle
added test for empty mapping
Michelle
Description From Last Updated

Since we're using both the key and the value, the items() iterator might be nicer than keys(): for ext, lexer_name ...

daviddavid

In the case where there's a configured lexer that matches an extension, but that doesn't exist in Pygments, we probably ...

daviddavid

Python actually has a really nice syntax feature where for has an else clause that gets executed in the case ...

daviddavid

W291 trailing whitespace

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E303 too many blank lines (2)

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

W291 trailing whitespace

reviewbotreviewbot

E122 continuation line missing indentation or outdented

reviewbotreviewbot

This doesn't seem valuable. Let's just use the one that we imported.

daviddavid

Please add a blank line between these.

daviddavid

Add a blank line here too.

daviddavid

The logging messages do their own format operation on the arguments, so we don't have to use % here. Let's ...

daviddavid

Ahh, I see now why you were defining the class local version. Django has a helper for this that you ...

daviddavid

W293 blank line contains whitespace

reviewbotreviewbot

E123 closing bracket does not match indentation of opening bracket's line

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

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

flake8

david
  1. Looks like a great start!

  2. reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
     
     
     
     

    Since we're using both the key and the value, the items() iterator might be nicer than keys():

    for ext, lexer_name in CUSTOM_PYGMENTS_LEXERS.items():
        ...
    
  3. In the case where there's a configured lexer that matches an extension, but that doesn't exist in Pygments, we probably should log an error so the user knows what went wrong.

  4. reviewboard/diffviewer/chunk_generator.py (Diff revision 1)
     
     
     
     

    Python actually has a really nice syntax feature where for has an else clause that gets executed in the case where nothing called break. So this could be combined:

    lexer = None
    
    for ext = CUSTOM_PYGMENTS_LEXERS.keys():
        # Try to instantiate lexer
        ...
    else:
        # Use guess_lexer_for_filename
    
  5. 
      
maubin
Review request changed

Summary:

-[WIP] Add a setting for mapping file extensions to syntax highlighters for diffs
+Add a setting for mapping file extensions to syntax highlighters for diffs

Testing Done:

~  

None yet.

  ~

Created the following unit tests in reviewboard.diffviewer.tests.test_raw_diff_chunk_generator:

  + - test_apply_pygments_with_custom_mapping() which tests if custom lexer mapping works and if it overrides the old behaviour of guessing
  + - test_apply_pygments_with_bad_custom_mapping() which tests if proper error is logged when the custom mapping maps an extension to a non existant lexer class

  +
  +

Ran all unit tests in reviewboard.diffviewer with success

Commits:

Summary Author
-
Add a setting for mapping file extensions to syntax highlighters for
Michelle
+
Add a setting for mapping file extensions to syntax highlighters for
Michelle
+
Uses for else clause and .items() instead of .keys() for searching the
Michelle
+
Logs an error if a mapped lexer does not exist in Pygments
Michelle
+
Removed trailing whitespace
Michelle
+
Properly imports mapping from settings.py
Michelle
+
fixed string formatting for logged error
Michelle
+
Made CUSTOM_PYGMENTS_LEXERS a class attribute (this makes it easier
Michelle
+
took out print statement from testing
Michelle
+
Added unit tests for apply_pygment, testing that default
Michelle
+
Modified doc formatting and changed default test to instead
Michelle

Diff:

Revision 2 (+225 -43)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
Review request changed

Commits:

Summary Author
-
Add a setting for mapping file extensions to syntax highlighters for
Michelle
-
Uses for else clause and .items() instead of .keys() for searching the
Michelle
-
Logs an error if a mapped lexer does not exist in Pygments
Michelle
-
Removed trailing whitespace
Michelle
-
Properly imports mapping from settings.py
Michelle
-
fixed string formatting for logged error
Michelle
-
Made CUSTOM_PYGMENTS_LEXERS a class attribute (this makes it easier
Michelle
-
took out print statement from testing
Michelle
-
Added unit tests for apply_pygment, testing that default
Michelle
-
Modified doc formatting and changed default test to instead
Michelle
+
Add a setting for mapping file extensions to syntax highlighters for
Michelle
+
Uses for else clause and .items() instead of .keys() for searching the
Michelle
+
Logs an error if a mapped lexer does not exist in Pygments
Michelle
+
Removed trailing whitespace
Michelle
+
Properly imports mapping from settings.py
Michelle
+
fixed string formatting for logged error
Michelle
+
Made CUSTOM_PYGMENTS_LEXERS a class attribute (this makes it easier
Michelle
+
took out print statement from testing
Michelle
+
Added unit tests for apply_pygment, testing that default
Michelle
+
Modified doc formatting and changed default test to instead
Michelle
+
fixed flake8 issues
Michelle

Diff:

Revision 3 (+227 -49)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
david
  1. 
      
  2. reviewboard/diffviewer/chunk_generator.py (Diff revision 4)
     
     
     

    This doesn't seem valuable. Let's just use the one that we imported.

  3. reviewboard/diffviewer/chunk_generator.py (Diff revision 4)
     
     
     

    Please add a blank line between these.

  4. reviewboard/diffviewer/chunk_generator.py (Diff revision 4)
     
     
     

    Add a blank line here too.

  5. reviewboard/diffviewer/chunk_generator.py (Diff revision 4)
     
     
     
     
     
     

    The logging messages do their own format operation on the arguments, so we don't have to use % here. Let's also make this message a little bit more explicit/terse:

    logger.error('Pygments lexer "%s" for "%s" files in '
                 'settings.CUSTOM_PYGMENTS_LEXERS was not found.',
                 lexer_name, ext)
    
  6. Ahh, I see now why you were defining the class local version. Django has a helper for this that you can use as a decorator: django.test.utils.override_settings:

    @override_settings(CUSTOM_PYGMENTS_LEXERS={'.md': 'LessCss'})
    def test_apply_pygments_with_custom_mapping(self):
        ...
    
  7. 
      
maubin
Review request changed

Commits:

Summary Author
-
Add a setting for mapping file extensions to syntax highlighters for
Michelle
-
Uses for else clause and .items() instead of .keys() for searching the
Michelle
-
Logs an error if a mapped lexer does not exist in Pygments
Michelle
-
Removed trailing whitespace
Michelle
-
Properly imports mapping from settings.py
Michelle
-
fixed string formatting for logged error
Michelle
-
Made CUSTOM_PYGMENTS_LEXERS a class attribute (this makes it easier
Michelle
-
took out print statement from testing
Michelle
-
Added unit tests for apply_pygment, testing that default
Michelle
-
Modified doc formatting and changed default test to instead
Michelle
-
fixed flake8 issues
Michelle
-
fixed one last flake8 indent issue
Michelle
+
Add a setting for mapping file extensions to syntax highlighters for
Michelle
+
Uses for else clause and .items() instead of .keys() for searching the
Michelle
+
Logs an error if a mapped lexer does not exist in Pygments
Michelle
+
Removed trailing whitespace
Michelle
+
Properly imports mapping from settings.py
Michelle
+
fixed string formatting for logged error
Michelle
+
Made CUSTOM_PYGMENTS_LEXERS a class attribute (this makes it easier
Michelle
+
took out print statement from testing
Michelle
+
Added unit tests for apply_pygment, testing that default
Michelle
+
Modified doc formatting and changed default test to instead
Michelle
+
fixed flake8 issues
Michelle
+
fixed one last flake8 indent issue
Michelle
+
better logging message, fixed formatting issues, removed mapping attribute and left it as constant from settings
Michelle

Diff:

Revision 5 (+236 -84)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
maubin
Review request changed

Summary:

-Add a setting for mapping file extensions to syntax highlighters for diffs
+[WIP] Add a setting for mapping file extensions to syntax highlighters for diffs

Commits:

Summary Author
-
Add a setting for mapping file extensions to syntax highlighters for
Michelle
-
Uses for else clause and .items() instead of .keys() for searching the
Michelle
-
Logs an error if a mapped lexer does not exist in Pygments
Michelle
-
Removed trailing whitespace
Michelle
-
Properly imports mapping from settings.py
Michelle
-
fixed string formatting for logged error
Michelle
-
Made CUSTOM_PYGMENTS_LEXERS a class attribute (this makes it easier
Michelle
-
took out print statement from testing
Michelle
-
Added unit tests for apply_pygment, testing that default
Michelle
-
Modified doc formatting and changed default test to instead
Michelle
-
fixed flake8 issues
Michelle
-
fixed one last flake8 indent issue
Michelle
-
better logging message, fixed formatting issues, removed mapping attribute and left it as constant from settings
Michelle
-
fixed a couple flake8 issues
Michelle
+
Add a setting for mapping file extensions to syntax highlighters for
Michelle
+
Uses for else clause and .items() instead of .keys() for searching the
Michelle
+
Logs an error if a mapped lexer does not exist in Pygments
Michelle
+
Removed trailing whitespace
Michelle
+
Properly imports mapping from settings.py
Michelle
+
fixed string formatting for logged error
Michelle
+
Made CUSTOM_PYGMENTS_LEXERS a class attribute (this makes it easier
Michelle
+
took out print statement from testing
Michelle
+
Added unit tests for apply_pygment, testing that default
Michelle
+
Modified doc formatting and changed default test to instead
Michelle
+
fixed flake8 issues
Michelle
+
fixed one last flake8 indent issue
Michelle
+
better logging message, fixed formatting issues, removed mapping attribute and left it as constant from settings
Michelle
+
fixed a couple flake8 issues
Michelle
+
moved mapping to siteconfig instead of settings.py
Michelle
+
initial form for mapping in diff viewer settings
Michelle
+
added test for empty mapping
Michelle

Diff:

Revision 7 (+334 -106)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

Loading...