From c0d72ba23675d1819d9f776b99cb02c59db39c8e Mon Sep 17 00:00:00 2001 From: Steve Howell Date: Thu, 2 Dec 2021 12:19:19 +0000 Subject: [PATCH] check-templates: Avoid duplicate tokenizing step. Now we only tokenize the file once, and we pass **validated** tokens to the pretty printer. There are a few reasons for this: * It obviously saves a lot of extra computation just in terms of tokenization. * It allows our validator to add fields to the Token objects that help the pretty printer. I also removed/tweaked a lot of legacy tests for pretty_print.py that were exercising bizarrely formatted HTML that we now simply ban during the validation phase. --- tools/check-templates | 12 +- tools/lib/pretty_print.py | 10 +- tools/lib/template_parser.py | 4 +- tools/tests/test_pretty_print.py | 278 +++++++------------------------ 4 files changed, 68 insertions(+), 236 deletions(-) diff --git a/tools/check-templates b/tools/check-templates index e8e985da85..5c634e0cca 100755 --- a/tools/check-templates +++ b/tools/check-templates @@ -115,10 +115,8 @@ def check_html_templates(templates: Iterable[str], all_dups: bool, fix: bool) -> sys.exit(1) for fn in templates: - validate(fn) - - for fn in templates: - if not validate_indent_html(fn, fix): + tokens = validate(fn) + if not validate_indent_html(fn, tokens, fix): sys.exit(1) @@ -127,10 +125,8 @@ def check_handlebar_templates(templates: Iterable[str], fix: bool) -> None: templates = [fn for fn in templates if fn.endswith(".hbs")] for fn in templates: - validate(fn) - - for fn in templates: - if not validate_indent_html(fn, fix): + tokens = validate(fn) + if not validate_indent_html(fn, tokens, fix): sys.exit(1) diff --git a/tools/lib/pretty_print.py b/tools/lib/pretty_print.py index c25a9b7d27..f36260069d 100644 --- a/tools/lib/pretty_print.py +++ b/tools/lib/pretty_print.py @@ -3,7 +3,7 @@ from typing import List, Optional, Set from zulint.printer import ENDC, GREEN -from .template_parser import Token, is_django_block_tag, tokenize +from .template_parser import Token, is_django_block_tag def requires_indent(line: str) -> bool: @@ -122,9 +122,7 @@ def get_exempted_lines(tokens: List[Token]) -> Set[int]: return exempted -def pretty_print_html(html: str) -> str: - tokens = tokenize(html) - +def pretty_print_html(html: str, tokens: List[Token]) -> str: exempted_lines = get_exempted_lines(tokens) tokens.reverse() @@ -207,10 +205,10 @@ def pretty_print_html(html: str) -> str: return "\n".join(formatted_lines) -def validate_indent_html(fn: str, fix: bool) -> bool: +def validate_indent_html(fn: str, tokens: List[Token], fix: bool) -> bool: with open(fn) as f: html = f.read() - phtml = pretty_print_html(html) + phtml = pretty_print_html(html, tokens) if not html.split("\n") == phtml.split("\n"): if fix: print(GREEN + f"Automatically fixing indentation for {fn}" + ENDC) diff --git a/tools/lib/template_parser.py b/tools/lib/template_parser.py index b134365ca7..9714dd70b4 100644 --- a/tools/lib/template_parser.py +++ b/tools/lib/template_parser.py @@ -316,7 +316,7 @@ def tag_flavor(token: Token) -> Optional[str]: raise AssertionError(f"tools programmer neglected to handle {kind} tokens") -def validate(fn: Optional[str] = None, text: Optional[str] = None) -> None: +def validate(fn: Optional[str] = None, text: Optional[str] = None) -> List[Token]: assert fn or text if fn is None: @@ -445,6 +445,8 @@ def validate(fn: Optional[str] = None, text: Optional[str] = None) -> None: ensure_matching_indentation(fn, tokens, lines) + return tokens + def ensure_matching_indentation(fn: str, tokens: List[Token], lines: List[str]) -> None: for token in tokens: diff --git a/tools/tests/test_pretty_print.py b/tools/tests/test_pretty_print.py index 419ccaa36e..497c9c021a 100644 --- a/tools/tests/test_pretty_print.py +++ b/tools/tests/test_pretty_print.py @@ -1,6 +1,7 @@ import unittest from tools.lib.pretty_print import pretty_print_html +from tools.lib.template_parser import validate # Note that GOOD_HTML isn't necessarily beautiful HTML. Apart # from adjusting indentation, we mostly leave things alone to @@ -10,8 +11,6 @@ BAD_HTML = """ - - @@ -20,19 +19,20 @@ BAD_HTML = """ -

Hello
world!

-

Goodbyeworld!

- - - - -
5
+

Hello
world!

+

Goodbyeworld!

+ + + + +
5
             print 'hello world'
-
{{ bla }}
+ role = "whatever">{{ bla }} + @@ -42,8 +42,6 @@ GOOD_HTML = """ - - @@ -64,7 +62,8 @@ GOOD_HTML = """
{{ bla }}
+ role = "whatever">{{ bla }} + @@ -72,9 +71,9 @@ GOOD_HTML = """ BAD_HTML1 = """ - - foobarfoobarfoobar - + + foobarfoobarfoobar + """ @@ -88,11 +87,11 @@ GOOD_HTML1 = """ BAD_HTML2 = """ - + {{# foobar area}} foobarfoobarfoobar - {{/ foobar area}} - + {{/ foobar}} + """ @@ -101,7 +100,7 @@ GOOD_HTML2 = """ {{# foobar area}} foobarfoobarfoobar - {{/ foobar area}} + {{/ foobar}} """ @@ -110,9 +109,9 @@ GOOD_HTML2 = """ BAD_HTML4 = """
- foo -

hello

- bar + foo +

hello

+ bar
""" @@ -126,13 +125,13 @@ GOOD_HTML4 = """ BAD_HTML5 = """
- foo - {{#if foobar}} - hello - {{else}} - bye - {{/if}} - bar + foo + {{#if foobar}} + hello + {{else}} + bye + {{/if}} + bar
""" @@ -150,7 +149,7 @@ GOOD_HTML5 = """ BAD_HTML6 = """
-

foobar

+

foobar

""" @@ -178,14 +177,14 @@ GOOD_HTML7 = """ BAD_HTML8 = """ {{#each test}} - {{#with this}} - {{#if foobar}} -
{{{test}}}
- {{/if}} - {{#if foobar2}} - {{> teststuff}} - {{/if}} - {{/with}} + {{#with this}} + {{#if foobar}} +
{{{test}}}
+ {{/if}} + {{#if foobar2}} + {{> teststuff}} + {{/if}} + {{/with}} {{/each}} """ @@ -262,11 +261,11 @@ GOOD_HTML10 = """ BAD_HTML11 = """
-
+
foobar -
+
+
-
""" @@ -280,167 +279,10 @@ GOOD_HTML11 = """
""" -BAD_HTML12 = """ -
-
-  

-    foobar
-    

-        

-

-
-
-""" -GOOD_HTML12 = """ -
-
-  

-    foobar
-    

-        

-

-
-
-""" - -BAD_HTML13 = """ -
- {{#if this.code}} -
 :{{this.name}}:
- {{else}} - {{#if this.is_realm_emoji}} - - {{else}} -
- {{/if}} - {{/if}} -
{{this.count}}
-
-""" - -GOOD_HTML13 = """ -
- {{#if this.code}} -
 :{{this.name}}:
- {{else}} - {{#if this.is_realm_emoji}} - - {{else}} -
- {{/if}} - {{/if}} -
{{this.count}}
-
-""" - -BAD_HTML14 = """ -
- {{#if this.code}} - 
Here goes some cool code.
- {{else}} -
- content of first div -
- content of second div. -
-
- {{/if}} -
-""" - -GOOD_HTML14 = """ -
- {{#if this.code}} - 
Here goes some cool code.
- {{else}} -
- content of first div -
- content of second div. -
-
- {{/if}} -
-""" - -BAD_HTML15 = """ -
- :thumbs_up: - :thumbs_up: - :thumbs_up: -
-""" - -GOOD_HTML15 = """ -
- :thumbs_up: - :thumbs_up: - :thumbs_up: -
-""" - -BAD_HTML16 = """ -
- {{> settings_checkbox - setting_name="realm_name_in_notifications" - is_checked=user_settings.realm_name_in_notifications - label=settings_label.realm_name_in_notifications}} -
-""" - -GOOD_HTML16 = """ -
- {{> settings_checkbox - setting_name="realm_name_in_notifications" - is_checked=user_settings.realm_name_in_notifications - label=settings_label.realm_name_in_notifications}} -
-""" - -BAD_HTML17 = """ -
- - -
- {{ bla }} -
-
-""" - -GOOD_HTML17 = """ -
- - -
- {{ bla }} -
-
-""" +def pretty_print(html: str) -> str: + tokens = validate(fn=None, text=html) + return pretty_print_html(html, tokens) class TestPrettyPrinter(unittest.TestCase): @@ -448,21 +290,15 @@ class TestPrettyPrinter(unittest.TestCase): self.assertEqual(a.split("\n"), b.split("\n")) def test_pretty_print(self) -> None: - self.compare(pretty_print_html(GOOD_HTML), GOOD_HTML) - self.compare(pretty_print_html(BAD_HTML), GOOD_HTML) - self.compare(pretty_print_html(BAD_HTML1), GOOD_HTML1) - self.compare(pretty_print_html(BAD_HTML2), GOOD_HTML2) - self.compare(pretty_print_html(BAD_HTML4), GOOD_HTML4) - self.compare(pretty_print_html(BAD_HTML5), GOOD_HTML5) - self.compare(pretty_print_html(BAD_HTML6), GOOD_HTML6) - self.compare(pretty_print_html(BAD_HTML7), GOOD_HTML7) - self.compare(pretty_print_html(BAD_HTML8), GOOD_HTML8) - self.compare(pretty_print_html(BAD_HTML9), GOOD_HTML9) - self.compare(pretty_print_html(BAD_HTML10), GOOD_HTML10) - self.compare(pretty_print_html(BAD_HTML11), GOOD_HTML11) - self.compare(pretty_print_html(BAD_HTML12), GOOD_HTML12) - self.compare(pretty_print_html(BAD_HTML13), GOOD_HTML13) - self.compare(pretty_print_html(BAD_HTML14), GOOD_HTML14) - self.compare(pretty_print_html(BAD_HTML15), GOOD_HTML15) - self.compare(pretty_print_html(BAD_HTML16), GOOD_HTML16) - self.compare(pretty_print_html(BAD_HTML17), GOOD_HTML17) + self.compare(pretty_print(GOOD_HTML), GOOD_HTML) + self.compare(pretty_print(BAD_HTML), GOOD_HTML) + self.compare(pretty_print(BAD_HTML1), GOOD_HTML1) + self.compare(pretty_print(BAD_HTML2), GOOD_HTML2) + self.compare(pretty_print(BAD_HTML4), GOOD_HTML4) + self.compare(pretty_print(BAD_HTML5), GOOD_HTML5) + self.compare(pretty_print(BAD_HTML6), GOOD_HTML6) + self.compare(pretty_print(BAD_HTML7), GOOD_HTML7) + self.compare(pretty_print(BAD_HTML8), GOOD_HTML8) + self.compare(pretty_print(BAD_HTML9), GOOD_HTML9) + self.compare(pretty_print(BAD_HTML10), GOOD_HTML10) + self.compare(pretty_print(BAD_HTML11), GOOD_HTML11)