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)

	reviewboard/settings.py
	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/tests/test_raw_diff_chunk_generator.py

	reviewboard/admin/siteconfig.py
	reviewboard/admin/forms/diff_settings.py
	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/tests/test_raw_diff_chunk_generator.py

	.browserslistrc
	AUTHORS
	dev-requirements.txt
	reviewboard/admin/form_widgets.py
	reviewboard/admin/siteconfig.py
	reviewboard/admin/forms/diff_settings.py
	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/tests/test_forms.py
	86 more

		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.

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.	810c268b04d766208982bd9cc5772fb472ab2e50	Michelle
Uses for else clause and .items() instead of .keys() for searching the mapping	a5a683ce294e75b13c7c4b600ab1c6b7a2d9416d	Michelle
Logs an error if a mapped lexer does not exist in Pygments	6da489bde81ca3eb9df84c3bb4e79425b18b7e45	Michelle
Removed trailing whitespace	8e396cb6f373d3336d411804d2973bb2f06680be	Michelle
Properly imports mapping from settings.py	23ec2199b6b867851f9d8d8163a85e9ba9898bce	Michelle
fixed string formatting for logged error	5785f25b427e1b18316db43ddcc98e02ba646892	Michelle
Made CUSTOM_PYGMENTS_LEXERS a class attribute (this makes it easier to modify during testing)	eefc9d52856d30d61963a6d34ebc76102b33bf2b	Michelle
took out print statement from testing	967ef3e281d39d468ef4d9c9f360f6a1e28087ec	Michelle
Added unit tests for apply_pygment, testing that default custom mapping works and that mapping to a non existant class logs an error	33139db632f2c36deb14cc9a493c4f33cb6754fe	Michelle
Modified doc formatting and changed default test to instead test if using a custom mapping overrides the guessing function to determine a lexer	224a9d9c751d4525937d3b4cae2f85a5f09d2e6a	Michelle
fixed flake8 issues	e6eed2477b86a382140fbe0d215819ea4a17c8a9	Michelle
fixed one last flake8 indent issue	9b1f2c9a68e2161a9d67cb1064ec09063e8aeb50	Michelle
better logging message, fixed formatting issues, removed mapping attribute and left it as constant from settings	211c72f95930609b6655ad0f06f229df884d2d62	Michelle
fixed a couple flake8 issues	ad32f6009c25e8d555c19622857422236ee46f5f	Michelle
moved mapping to siteconfig instead of settings.py	d7549e8107b35ceae203e9099b2a323bd7e89042	Michelle
initial form for mapping in diff viewer settings	09b9854b63f1c67e6684c300b7c9fd971a65d175	Michelle
added test for empty mapping	a36c2ee69209caa2053c1ea80431762a97cf9379	Michelle
create LexersMappingWidget and using in diff_settings.py for now to test	dfdc4468076bf6d899533efc6fb3c4b9f1088394	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.	810c268b04d766208982bd9cc5772fb472ab2e50	Michelle
Uses for else clause and .items() instead of .keys() for searching the mapping	a5a683ce294e75b13c7c4b600ab1c6b7a2d9416d	Michelle
Logs an error if a mapped lexer does not exist in Pygments	6da489bde81ca3eb9df84c3bb4e79425b18b7e45	Michelle
Removed trailing whitespace	8e396cb6f373d3336d411804d2973bb2f06680be	Michelle
Properly imports mapping from settings.py	23ec2199b6b867851f9d8d8163a85e9ba9898bce	Michelle
fixed string formatting for logged error	5785f25b427e1b18316db43ddcc98e02ba646892	Michelle
Made CUSTOM_PYGMENTS_LEXERS a class attribute (this makes it easier to modify during testing)	eefc9d52856d30d61963a6d34ebc76102b33bf2b	Michelle
took out print statement from testing	967ef3e281d39d468ef4d9c9f360f6a1e28087ec	Michelle
Added unit tests for apply_pygment, testing that default custom mapping works and that mapping to a non existant class logs an error	33139db632f2c36deb14cc9a493c4f33cb6754fe	Michelle
Modified doc formatting and changed default test to instead test if using a custom mapping overrides the guessing function to determine a lexer	224a9d9c751d4525937d3b4cae2f85a5f09d2e6a	Michelle
fixed flake8 issues	e6eed2477b86a382140fbe0d215819ea4a17c8a9	Michelle
fixed one last flake8 indent issue	9b1f2c9a68e2161a9d67cb1064ec09063e8aeb50	Michelle
better logging message, fixed formatting issues, removed mapping attribute and left it as constant from settings	211c72f95930609b6655ad0f06f229df884d2d62	Michelle
fixed a couple flake8 issues	ad32f6009c25e8d555c19622857422236ee46f5f	Michelle
moved mapping to siteconfig instead of settings.py	d7549e8107b35ceae203e9099b2a323bd7e89042	Michelle
initial form for mapping in diff viewer settings	09b9854b63f1c67e6684c300b7c9fd971a65d175	Michelle
added test for empty mapping	a36c2ee69209caa2053c1ea80431762a97cf9379	Michelle
create LexersMappingWidget and using in diff_settings.py for now to test	dfdc4468076bf6d899533efc6fb3c4b9f1088394	Michelle
create tests for lexers mapping widget	2d37e23bb79913a72a2e2c580d2023e73d7449c7	Michelle

	docs/manual/admin/configuration/diffviewer-settings.rst
	reviewboard/admin/form_widgets.py
	reviewboard/admin/siteconfig.py
	reviewboard/admin/forms/diff_settings.py
	reviewboard/admin/tests/test_lexers_mapping_widget.py
	reviewboard/diffviewer/chunk_generator.py
	reviewboard/diffviewer/tests/test_raw_diff_chunk_generator.py

	Summary	ID	Author
	resolved merge conflicts from reviewboard-5.0.x updates	4095f339e8f0d2f731c002a4188be81e0f6cd167	Michelle
	resolved merge conflicts from reviewboard-5.0.x updates	c9a53a82a075528117ee4f95a14a0c88a2ab8be1	Michelle

~		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
	~	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
	~	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.
	~	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.
-		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	ID	Author
	resolved merge conflicts from reviewboard-5.0.x updates	326476223e1163d83986d3857526c5f373f843fc	Michelle
	resolved merge conflicts from reviewboard-5.0.x updates	97c7be5b4bb62f77b72187f28879a7d77089ef10	Michelle

	Summary	ID	Author
	resolved merge conflicts from reviewboard-5.0.x updates	d9de4e339566468c528b53f6f36ede7f7bb1ec25	Michelle
	resolved merge conflicts from reviewboard-5.0.x updates	930128881e245f00cdc9f918b56bdf483d734336	Michelle

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

Information

Reviewers

flake8

Checks run (1 failed, 1 succeeded)

flake8

Checks run (1 failed, 1 succeeded)

flake8

Checks run (2 succeeded)

Checks run (1 failed, 1 succeeded)

flake8

Checks run (2 succeeded)

Checks run (1 failed, 1 succeeded)

flake8

Checks run (1 failed, 1 succeeded)

flake8

Checks run (1 failed, 1 succeeded)

flake8

Checks run (1 failed, 1 succeeded)

flake8

Checks run (1 failed, 1 succeeded)

flake8

Checks run (1 failed, 1 succeeded)

flake8

Checks run (2 succeeded)

Checks run (2 succeeded)

Checks run (2 succeeded)

Checks run (1 failed, 1 succeeded)

flake8

Checks run (2 succeeded)

Checks run (2 succeeded)

Checks run (2 succeeded)

Checks run (2 succeeded)

Checks run (2 succeeded)

Checks run (2 succeeded)