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

Adds reviewboard.admin.siteconfig.diffviewer_custom_pygments_lexers,
a mapping of file extensions to pygments lexers. 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.

Adds reviewboard.admin.form_widgets.LexersMappingWidget which partly
handles the UI for modifying this mapping.

Created unit tests for the mapping in
reviewboard.diffviewer.tests.test_raw_diff_chunk_generator.
Ran all unit tests in reviewboard.diffviewer with success

Created the unit tests for the LexersMappingWidget in
reviewboard.admin.tests.test_lexers_mapping_widget.
Ran all unit tests in reviewboard.admin.tests 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
create LexersMappingWidget and using in diff_settings.py for now to test
Michelle
create tests for lexers mapping widget
Michelle
testing listeditwidget with custom pygments lexers setting
Michelle
add documentation and finalize code
Michelle
no need for local site tests
Michelle
skip extension if its empty string since it would match any file
Michelle
fix some flake8 issues
Michelle
address review feedback
Michelle
address feedback
Michelle
fixed styling issues and uses siteconfig context
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

W292 no newline at end of file

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

W293 blank line contains whitespace

reviewbotreviewbot

Let's try to shorten this to be a single line. Additional detail can be added in the next paragraph if ...

daviddavid

Leading underscore isn't needed here.

daviddavid

Dedent 4 spaces.

daviddavid

Dedent 4 spaces.

daviddavid

We should probably clarify in the help text that this is for pygments so people can find the appropriate documentation.

daviddavid

Let's use parens and string concatenation instead of the triple-quoted string here: correct_html = ( '<input type...' '<select name...' )

daviddavid

Can wrap this a little bit nicer: correct_html += format_html( '...') That said, string concatenation in Python isn't particularly efficient. ...

daviddavid

Same comments here as in above method.

daviddavid

Keep imports in alphabetical order.

daviddavid

Let's swap these so they're alphabetical

daviddavid

Add a blank line here.

daviddavid

Please alphabetize.

daviddavid

Add a blank line before these. Let's also put c before j

daviddavid

Alphabetize.

daviddavid

Add a blank line in here.

daviddavid

Add a blank line between these two.

daviddavid

This comment just says exactly what the code says, so it's not adding much information. Let's get rid of it.

daviddavid

This should go in its own import section above the reviewboard imports.

daviddavid

We have a context manager that might dramatically simplify this test class. It will do a temporary override of the ...

daviddavid
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

maubin
Review request changed

Change Summary:

Create LexersMappingWidget and using it in diff settings to test for now

Testing Done:

   

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

  ~ - 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
  + - test_apply_pygments_with_empty_custom_mapping() which tests if apply_pygments still works as expected with an empty custom mapping.

   
   

Ran all unit tests in reviewboard.diffviewer with success

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
-
moved mapping to siteconfig instead of settings.py
Michelle
-
initial form for mapping in diff viewer settings
Michelle
-
added test for empty mapping
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
+
create LexersMappingWidget and using in diff_settings.py for now to test
Michelle

Diff:

Revision 8 (+1274 -1279)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
Review request changed

Change Summary:

took out changes from bad rebasing

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
-
moved mapping to siteconfig instead of settings.py
Michelle
-
initial form for mapping in diff viewer settings
Michelle
-
added test for empty mapping
Michelle
-
create LexersMappingWidget and using in diff_settings.py for now to test
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
+
create LexersMappingWidget and using in diff_settings.py for now to test
Michelle

Diff:

Revision 9 (+504 -114)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
Review request changed

Change Summary:

Add tests for LexersMappingWidget

Description:

   

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.

  +
  +

Adds reviewboard.admin.form_widgets.LexersMappingWidget which partly handles

  + the UI for maintaining this mapping.

Testing Done:

   

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
    - test_apply_pygments_with_empty_custom_mapping() which tests if apply_pygments still works as expected with an empty custom mapping.

   
   

Ran all unit tests in reviewboard.diffviewer with success

  +
  +

Created the unit tests for the LexersMappingWidget in reviewboard.admin.tests.test_lexers_mapping_widget.

  + Ran all unit tests in reviewboard.admin.tests with success.

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
-
moved mapping to siteconfig instead of settings.py
Michelle
-
initial form for mapping in diff viewer settings
Michelle
-
added test for empty mapping
Michelle
-
create LexersMappingWidget and using in diff_settings.py for now to test
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
+
create LexersMappingWidget and using in diff_settings.py for now to test
Michelle
+
create tests for lexers mapping widget
Michelle

Diff:

Revision 10 (+809 -115)

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
-
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
-
create LexersMappingWidget and using in diff_settings.py for now to test
Michelle
-
create tests for lexers mapping widget
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
+
create LexersMappingWidget and using in diff_settings.py for now to test
Michelle
+
create tests for lexers mapping widget
Michelle
+
testing listeditwidget with custom pygments lexers setting
Michelle

Diff:

Revision 11 (+815 -123)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
Review request changed

Change Summary:

Everything is done and tested, except for handling local site stuff

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
-
moved mapping to siteconfig instead of settings.py
Michelle
-
initial form for mapping in diff viewer settings
Michelle
-
added test for empty mapping
Michelle
-
create LexersMappingWidget and using in diff_settings.py for now to test
Michelle
-
create tests for lexers mapping widget
Michelle
-
testing listeditwidget with custom pygments lexers setting
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
+
create LexersMappingWidget and using in diff_settings.py for now to test
Michelle
+
create tests for lexers mapping widget
Michelle
+
testing listeditwidget with custom pygments lexers setting
Michelle
+
add documentation and finalize code
Michelle

Diff:

Revision 12 (+824 -136)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
david
  1. 
      
  2. reviewboard/admin/form_widgets.py (Diff revision 13)
     
     
     

    Let's try to shorten this to be a single line. Additional detail can be added in the next paragraph if necessary.

  3. reviewboard/admin/form_widgets.py (Diff revision 13)
     
     

    Leading underscore isn't needed here.

  4. reviewboard/admin/form_widgets.py (Diff revision 13)
     
     

    Dedent 4 spaces.

  5. reviewboard/admin/form_widgets.py (Diff revision 13)
     
     

    Dedent 4 spaces.

  6. reviewboard/admin/forms/diff_settings.py (Diff revision 13)
     
     
     

    We should probably clarify in the help text that this is for pygments so people can find the appropriate documentation.

  7. Let's use parens and string concatenation instead of the triple-quoted string here:

    correct_html = (
        '<input type...'
        '<select name...'
    )
    
  8. Can wrap this a little bit nicer:

    correct_html += format_html(
        '...')
    

    That said, string concatenation in Python isn't particularly efficient. The usual convention is to build a list and join all at once:

    correct_html_parts = [
        '<input ...',
        '<select...',
    ]
    
    for lex in get_all_lexers():
        ...
        correct_html_parts.append(format_html(
            '...'))
    
    correct_html_parts.append('</select>')
    correct_html = ''.join(correct_html_parts)
    
  9. Same comments here as in above method.

  10. reviewboard/diffviewer/chunk_generator.py (Diff revision 13)
     
     
     
     

    Keep imports in alphabetical order.

  11. reviewboard/diffviewer/chunk_generator.py (Diff revision 13)
     
     
     

    Let's swap these so they're alphabetical

  12. 
      
maubin
david
  1. 
      
  2. reviewboard/admin/form_widgets.py (Diff revision 14)
     
     
     

    Add a blank line here.

  3. reviewboard/admin/forms/diff_settings.py (Diff revision 14)
     
     
     
     

    Please alphabetize.

  4. reviewboard/admin/forms/diff_settings.py (Diff revision 14)
     
     
     

    Add a blank line before these. Let's also put c before j

  5. Alphabetize.

  6. Add a blank line in here.

  7. Add a blank line between these two.

  8. This comment just says exactly what the code says, so it's not adding much information. Let's get rid of it.

  9. This should go in its own import section above the reviewboard imports.

  10. reviewboard/diffviewer/tests/test_raw_diff_chunk_generator.py (Diff revision 14)
     
     
     
     
     
     
     
     

    We have a context manager that might dramatically simplify this test class. It will do a temporary override of the siteconfig and then restore the old settings when that scope is exited. Can you try this and see if it does the right thing?

    ```python
    settings = {
    'diffviewer_custom_pygments_lexers': {'.md', 'LessCss'},
    }

    with self.siteconfig_settings(settings):
    chunk_generator = ....

    1. Tried it out and ran into this error.

      ERROR: Testing RawDiffChunkGenerator._apply_pygments with a custom lexer mapping
      ----------------------------------------------------------------------
      Traceback (most recent call last):
        File "/home/maubin/reviewboard/src/rb-master/reviewboard/reviewboard/diffviewer/tests/test_raw_diff_chunk_generator.py", line 478, in test_apply_pygments_with_custom_mapping
          with self.siteconfig_settings(settings):
        File "/home/maubin/.pyenv/versions/3.9.7/lib/python3.9/contextlib.py", line 119, in __enter__
          return next(self.gen)
        File "/home/maubin/reviewboard/src/rb-master/reviewboard/reviewboard/testing/testcase.py", line 1904, in siteconfig_settings
          with super(TestCase, self).siteconfig_settings(settings):
        File "/home/maubin/.pyenv/versions/3.9.7/lib/python3.9/contextlib.py", line 119, in __enter__
          return next(self.gen)
        File "/home/maubin/reviewboard/src/rb-master/djblets/djblets/testing/testcases.py", line 162, in siteconfig_settings
          siteconfig.save()
        File "/home/maubin/reviewboard/src/rb-master/djblets/djblets/siteconfig/models.py", line 309, in save
          super(SiteConfiguration, self).save(**kwargs)
        File "/home/maubin/envs/reviewboard-dev/lib/python3.9/site-packages/django/db/models/base.py", line 807, in save
          self.save_base(using=using, force_insert=force_insert,
        File "/home/maubin/envs/reviewboard-dev/lib/python3.9/site-packages/django/db/models/base.py", line 838, in save_base
          updated = self._save_table(raw, cls, force_insert, force_update, using, update_fields)
        File "/home/maubin/envs/reviewboard-dev/lib/python3.9/site-packages/django/db/models/base.py", line 901, in _save_table
          values = [(f, None, (getattr(self, f.attname) if raw else f.pre_save(self, False)))
        File "/home/maubin/envs/reviewboard-dev/lib/python3.9/site-packages/django/db/models/base.py", line 901, in <listcomp>
          values = [(f, None, (getattr(self, f.attname) if raw else f.pre_save(self, False)))
        File "/home/maubin/reviewboard/src/rb-master/djblets/djblets/db/fields/json_field.py", line 244, in pre_save
          return self.dumps(getattr(model_instance, self.attname, None))
        File "/home/maubin/reviewboard/src/rb-master/djblets/djblets/db/fields/json_field.py", line 352, in dumps
          return self.encoder.encode(data)
        File "/home/maubin/.pyenv/versions/3.9.7/lib/python3.9/json/encoder.py", line 199, in encode
          chunks = self.iterencode(o, _one_shot=True)
        File "/home/maubin/.pyenv/versions/3.9.7/lib/python3.9/json/encoder.py", line 257, in iterencode
          return _iterencode(o, 0)
        File "/home/maubin/reviewboard/src/rb-master/djblets/djblets/util/serializers.py", line 77, in default
          return super(DjbletsJSONEncoder, self).default(obj)
        File "/home/maubin/envs/reviewboard-dev/lib/python3.9/site-packages/django/core/serializers/json.py", line 124, in default
          return super(DjangoJSONEncoder, self).default(o)
        File "/home/maubin/.pyenv/versions/3.9.7/lib/python3.9/json/encoder.py", line 179, in default
          raise TypeError(f'Object of type {o.__class__.__name__} '
      TypeError: Object of type set is not JSON serializable
      -------------------- >> begin captured logging << --------------------
      reviewboard.admin.siteconfig: ERROR: Could not load siteconfig: An error occurred in the current transaction. You can't execute queries until the end of the 'atomic' block.
      --------------------- >> end captured logging << ---------------------
      
    2. Looks like I had typoed it. Try changing {'.md', 'LessCss'} to {'.md': 'LessCss'} (, -> :)

    3. Oh oops, it does work now!

  11. 
      
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
-
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
-
create LexersMappingWidget and using in diff_settings.py for now to test
Michelle
-
create tests for lexers mapping widget
Michelle
-
testing listeditwidget with custom pygments lexers setting
Michelle
-
add documentation and finalize code
Michelle
-
no need for local site tests
Michelle
-
skip extension if its empty string since it would match any file
Michelle
-
fix some flake8 issues
Michelle
-
address review feedback
Michelle
-
address feedback
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
+
create LexersMappingWidget and using in diff_settings.py for now to test
Michelle
+
create tests for lexers mapping widget
Michelle
+
testing listeditwidget with custom pygments lexers setting
Michelle
+
add documentation and finalize code
Michelle
+
no need for local site tests
Michelle
+
skip extension if its empty string since it would match any file
Michelle
+
fix some flake8 issues
Michelle
+
address review feedback
Michelle
+
address feedback
Michelle
+
fixed styling issues and uses siteconfig context
Michelle

Diff:

Revision 15 (+889 -265)

Show changes

Checks run (2 succeeded)

flake8 passed.
JSHint passed.
Loading...