Summary

Add a setting for mapping file extensions to syntax highlighters for diffs

Review Request #11821 — Created Sept. 24, 2021 and submitted June 3, 2022, 12:43 p.m.

Information

Owner

maubin

Repository

Review Board

Branch

release-5.0.x

Bugs

Depends On

~~11858~~

Blocks

11868

Reviewers

Groups

reviewboard

People

Description

Previously, we guess what syntax highlighting to apply to files in the diff
viewer given their data and file extension. Sometimes this guess would be wrong,
and we have no way of setting what type of syntax highlighting gets applied to
files. This change adds a setting for mapping file extensions to pygments
lexers (the syntax highlighters), namely
reviewboard.admin.siteconfig.diffviewer_custom_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.

Testing Done


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.

Commits

Summary	ID	Author
resolved merge conflicts from reviewboard-5.0.x updates	aac35dd214d59c3a66996bae103eaee99691960d	Michelle

Issues

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 …	david	Sept. 28, 2021, 9:52 p.m.
In the case where there's a configured lexer that matches an extension, but that doesn't exist in Pygments, we probably …	david	Sept. 29, 2021, 12:30 p.m.
Python actually has a really nice syntax feature where for has an else clause that gets executed in the case …	david	Sept. 28, 2021, 10:05 p.m.
W291 trailing whitespace	reviewbot	Sept. 29, 2021, 12:31 p.m.
E122 continuation line missing indentation or outdented	reviewbot	Oct. 5, 2021, 2:16 p.m.
W293 blank line contains whitespace	reviewbot	Oct. 5, 2021, 2:09 p.m.
E303 too many blank lines (2)	reviewbot	Oct. 5, 2021, 2:10 p.m.
E122 continuation line missing indentation or outdented	reviewbot	Oct. 5, 2021, 2:11 p.m.
W293 blank line contains whitespace	reviewbot	Oct. 5, 2021, 2:12 p.m.
W291 trailing whitespace	reviewbot	Oct. 5, 2021, 2:12 p.m.
W291 trailing whitespace	reviewbot	Oct. 5, 2021, 2:12 p.m.
E122 continuation line missing indentation or outdented	reviewbot	Oct. 5, 2021, 2:18 p.m.
This doesn't seem valuable. Let's just use the one that we imported.	david	Oct. 6, 2021, 1:50 p.m.
Please add a blank line between these.	david	Oct. 6, 2021, 1:58 p.m.
Add a blank line here too.	david	Oct. 6, 2021, 1:59 p.m.
The logging messages do their own format operation on the arguments, so we don't have to use % here. Let's …	david	Oct. 6, 2021, 2:02 p.m.
Ahh, I see now why you were defining the class local version. Django has a helper for this that you …	david	Oct. 6, 2021, 2:17 p.m.
W293 blank line contains whitespace	reviewbot	Oct. 6, 2021, 5:49 p.m.
E123 closing bracket does not match indentation of opening bracket's line	reviewbot	Oct. 6, 2021, 5:49 p.m.
E501 line too long (84 > 79 characters)	reviewbot	Nov. 3, 2021, 8:45 p.m.
W293 blank line contains whitespace	reviewbot	Nov. 3, 2021, 8:45 p.m.
W292 no newline at end of file	reviewbot	Nov. 3, 2021, 9:22 p.m.
E501 line too long (84 > 79 characters)	reviewbot	Nov. 3, 2021, 9:22 p.m.
W293 blank line contains whitespace	reviewbot	Nov. 3, 2021, 9:22 p.m.
W292 no newline at end of file	reviewbot	Nov. 9, 2021, 12:37 p.m.
E501 line too long (84 > 79 characters)	reviewbot	Nov. 9, 2021, 12:37 p.m.
W293 blank line contains whitespace	reviewbot	Nov. 9, 2021, 12:37 p.m.
E501 line too long (84 > 79 characters)	reviewbot	Nov. 16, 2021, 12:03 p.m.
W293 blank line contains whitespace	reviewbot	Nov. 16, 2021, 12:03 p.m.
E501 line too long (84 > 79 characters)	reviewbot	Nov. 17, 2021, 2:04 p.m.
W293 blank line contains whitespace	reviewbot	Nov. 17, 2021, 2:04 p.m.
E501 line too long (84 > 79 characters)	reviewbot	Nov. 22, 2021, 10:06 p.m.
W293 blank line contains whitespace	reviewbot	Nov. 22, 2021, 10:07 p.m.
Let's try to shorten this to be a single line. Additional detail can be added in the next paragraph if …	david	Dec. 1, 2021, 10:02 p.m.
Leading underscore isn't needed here.	david	Dec. 1, 2021, 10:02 p.m.
Dedent 4 spaces.	david	Dec. 1, 2021, 10:03 p.m.
Dedent 4 spaces.	david	Dec. 1, 2021, 10:03 p.m.
We should probably clarify in the help text that this is for pygments so people can find the appropriate documentation.	david	Nov. 29, 2021, 2:47 p.m.
Let's use parens and string concatenation instead of the triple-quoted string here: correct_html = ( '<input type...' '<select name...' )	david	Dec. 1, 2021, 10:05 p.m.
Can wrap this a little bit nicer: correct_html += format_html( '...') That said, string concatenation in Python isn't particularly efficient. …	david	Dec. 1, 2021, 10:08 p.m.
Same comments here as in above method.	david	Dec. 1, 2021, 10:11 p.m.
Keep imports in alphabetical order.	david	Dec. 1, 2021, 10:12 p.m.
Let's swap these so they're alphabetical	david	Dec. 1, 2021, 10:12 p.m.
Add a blank line here.	david	Jan. 5, 2022, 6:29 p.m.
Please alphabetize.	david	Jan. 5, 2022, 6:30 p.m.
Add a blank line before these. Let's also put c before j	david	Jan. 5, 2022, 7:22 p.m.
Alphabetize.	david	Jan. 5, 2022, 7:23 p.m.
Add a blank line in here.	david	Jan. 5, 2022, 7:24 p.m.
Add a blank line between these two.	david	Jan. 5, 2022, 7:26 p.m.
This comment just says exactly what the code says, so it's not adding much information. Let's get rid of it.	david	Jan. 5, 2022, 7:26 p.m.
This should go in its own import section above the reviewboard imports.	david	Jan. 5, 2022, 7:27 p.m.
We have a context manager that might dramatically simplify this test class. It will do a temporary override of the …	david	Jan. 6, 2022, 12:46 p.m.
F811 redefinition of unused 'render_to_string' from line 6	reviewbot	May 17, 2022, 1:46 p.m.
Add a blank line between these two.	david	May 23, 2022, 9:35 p.m.
This needs to just be a single line. If we need more detail, we can add another paragraph.	david	May 23, 2022, 9:35 p.m.
should I add a "Version added: 5.0.0" in here?	maubin	June 1, 2022, 1:37 p.m.
Since the entire class is added in 5.0, I don't think we need version specifiers for each method inside it. …	david	June 1, 2022, 3:23 p.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/settings.py (Diff revision 1)
The issue has been resolved. Show all issues
```
W291 trailing whitespace
```

```
Looks like a great start!
```

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

The issue has been resolved. Show all issues

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():
    ...

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

The issue has been resolved. Show all issues

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.

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

The issue has been resolved. Show all issues

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

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	ID	Author
Add a setting for mapping file extensions to syntax highlighters for diffs 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.	3e4ddeda899f0e5d97f02ed070d7525662ee33c5	Michelle
Add a setting for mapping file extensions to syntax highlighters for diffs 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.	3e4ddeda899f0e5d97f02ed070d7525662ee33c5	Michelle
Uses for else clause and .items() instead of .keys() for searching the mapping	97a6bd3547ff2c3af0708640538d75ef4a55f1c8	Michelle
Logs an error if a mapped lexer does not exist in Pygments	13f279fa09f30803a02621e13192942b95e80460	Michelle
Removed trailing whitespace	d27cc3d581a53a7fd414db5efc820cba92ba9fb3	Michelle
Properly imports mapping from settings.py	8eb445508de1a38aae178bc59477b585f681bbd6	Michelle
fixed string formatting for logged error	dafacdae4c204636121480ab9b3821ffec9ff889	Michelle
Made CUSTOM_PYGMENTS_LEXERS a class attribute (this makes it easier to modify during testing)	45754bd19bf4ae4ae6f9582e725557ddf0e3c976	Michelle
took out print statement from testing	5fb349b3f4d0adb8a4f0c0b25d3df3611a446220	Michelle
Added unit tests for apply_pygment, testing that default custom mapping works and that mapping to a non existant class logs an error	c29591ba2ab98206ea65f12b248826d2e5ad5eac	Michelle
Modified doc formatting and changed default test to instead test if using a custom mapping overrides the guessing function to determine a lexer	f853203d6bcadd51d4e9dcfe9561607034023ab7	Michelle

Diff:

Revision 2 (+225 -43)

Add a setting for mapping file extensions to syntax highlighters for diffs

Information

Reviewers

flake8

Summary:

Testing Done:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (2 succeeded)

Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Testing Done:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Description:

Testing Done:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Commits:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Change Summary:

Summary:

Description:

Testing Done:

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Branch:

Depends On:

Diff:

Checks run (1 failed, 1 succeeded)

flake8

Commits:

Diff:

Checks run (2 succeeded)

Commits:

Diff: