Add ability to pretty-print HTML (lib only). #3428

adnrs96 · 2017-01-22T15:02:06Z

This PR extends the work being done here in PR#1778.
@showell has already implemented a tool to prettify HTML in above PR. I have extended it to deal with cases in which HTML tags are not the start of line and added support for handlebar tags.
This will add ability to check our handlebar templates for 4 Space Indentation and will work forward to enforce 4 space indents by tightening our linters. This has been discussed in this issue #1661.

smarx · 2017-01-22T15:02:08Z

Automated message from Dropbox CLA bot

@adnrs96, it looks like you've already signed the Dropbox CLA. Thanks!

showell · 2017-01-22T15:22:17Z

Hmmm...I'm not sure the two-pass approach will work for html/handlebars. I believe you need to check for indentation in one single pass and treat both tags the same way.

adnrs96 · 2017-01-22T15:40:34Z

@showell Thank you for taking a look.
Actually I thought it this way that at a given time the token could be either a html tag or a handlebar tag.
Now if its a html tag then nothing would be done when we call using 'handlebar' and when we have a handlebar token nothing would happen when we call using 'html'. I actually was trying to reduce redundancy in code by doing it this way. Now when I think of it, a conditional call would have been better.
Wouldn't it be too much code redundancy if we make it in a linear if else fashion?
PS: I pushed another commit to add conditional call please take a look.

showell · 2017-01-22T15:44:55Z

Huh, I misread the original code. But I still think you'd be better off having determine_offsets do something like if token in ('html_start', 'handlebars_start')

adnrs96 · 2017-01-22T15:47:58Z

yeah that sounds more pythonic, I will go with that.

adnrs96 · 2017-01-22T16:46:23Z

I made the changes suggested. Please take a look.
Also should I start cleaning up the handlebar templates as you suggested to do.

timabbott · 2017-01-22T23:44:19Z

I think structurally, we'll want to merge the tooling before merging migrations for individual templates, but it's probably useful to do a couple handlebars templates to test the procedure.

showell · 2017-01-26T22:17:51Z

tools/lib/pretty_print.py

+                else:
+                    for line_num in range(start_line + 1, end_line):
+                        if line_num not in offsets:
+                            offsets[line_num] = offset


Where does offset get set for this line to work?

My bad it should have been
info['offset'] here.
Will correct it.

tommyip

Pretty good! Just a few comments.
Also just for having consistent styling, can you have 2 spaces before inline comments?

tommyip · 2017-02-03T17:33:02Z

tools/lib/pretty_print.py

+    # Now that we have all of our offsets calculated, we can just
+    # join all our lines together, fixing up offsets as needed.
+    formatted_lines = []
+    for i, line in enumerate(html.split('\n')):


What if there is a literal \n in the HTML? Like:

<div class='codeblock'> print('\n') </div>

will need to see into this

no that won't be a problem i guess.
\n within a file is read as \\n\

tommyip · 2017-02-03T17:36:05Z

tools/tests/test_pretty_print.py

+        self.compare(pretty_print_html(BAD_HTML2), GOOD_HTML2)
+        self.compare(pretty_print_html(BAD_HTML3), GOOD_HTML3)
+        self.compare(pretty_print_html(BAD_HTML4), GOOD_HTML4)
+        self.compare(pretty_print_html(BAD_HTML5), GOOD_HTML5)


Maybe add a testcase for a normal HTML file with correct indentation to check if it changes anything.

Also need trailing new line.

ohk that test makes sense. I will add it.

tommyip · 2017-02-03T17:36:31Z

tools/lib/pretty_print.py

+                assert line.strip() == pretty_line.strip()
+        formatted_lines.append(pretty_line)
+
+    return '\n'.join(formatted_lines)


Need a trailing new line here.

tommyip · 2017-02-03T17:42:08Z

tools/tests/test_pretty_print.py

+        </pre>
+        <div class = "foo"
+            id = "bar"
+             role = "whatever">{{ bla }}</div>


Is there any reason for the extra space here?

@tommyip We want the beautifier to be not overly aggressive about fixing every little quirk of the HTML, in case the original coders had some reason to format things the way we did. That decision may change, though; we might want to move toward a more opinionated formatter.

@showell

In This commit we extend the work being done by @showell in PR#1778 to develop a tool to pretty print html and our handlebar templates in order to enforce our style convention of 4 Space indentation in templates. This commit introduces following changes: * Fix Py3 Compatibility. * Add ability to prettify in cases when html tags are not the starting of a line and addition of test cases for it. * Add ability to lint handlebar tags and add test cases for it. * Add {{else}} as special case of indent. * Add test cases in general to testing new tool. @showell Helped me throughout and reviewed this commit. Fixes zulip#1778

adnrs96 force-pushed the FIX-1659 branch from da45868 to 0afc8fe Compare January 22, 2017 16:27

adnrs96 force-pushed the FIX-1659 branch 6 times, most recently from 50de6cf to c2da4b5 Compare January 26, 2017 22:08

showell reviewed Jan 26, 2017

View reviewed changes

adnrs96 force-pushed the FIX-1659 branch from c2da4b5 to 01e271f Compare February 3, 2017 16:54

tommyip reviewed Feb 3, 2017

View reviewed changes

adnrs96 force-pushed the FIX-1659 branch 2 times, most recently from 67d720f to ceaad09 Compare February 7, 2017 18:33

adnrs96 mentioned this pull request Feb 13, 2017

Clean handlebar templates to use 4 Space Indent. #3658

Merged

adnrs96 force-pushed the FIX-1659 branch 5 times, most recently from fc0e117 to 0b0ffeb Compare February 15, 2017 05:35

adnrs96 force-pushed the FIX-1659 branch from 0b0ffeb to 79f0845 Compare February 15, 2017 05:36

showell merged commit e84cf7b into zulip:master Feb 15, 2017

adnrs96 changed the title ~~[WIP-1661] Add ability to pretty-print HTML (lib only).~~ Add ability to pretty-print HTML (lib only). Feb 15, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to pretty-print HTML (lib only). #3428

Add ability to pretty-print HTML (lib only). #3428

adnrs96 commented Jan 22, 2017 •

edited

smarx commented Jan 22, 2017

showell commented Jan 22, 2017

adnrs96 commented Jan 22, 2017

showell commented Jan 22, 2017

adnrs96 commented Jan 22, 2017

adnrs96 commented Jan 22, 2017

timabbott commented Jan 22, 2017

showell Jan 26, 2017

adnrs96 Jan 26, 2017

tommyip left a comment

tommyip Feb 3, 2017

adnrs96 Feb 3, 2017

adnrs96 Feb 3, 2017 •

edited

tommyip Feb 3, 2017

tommyip Feb 3, 2017

adnrs96 Feb 3, 2017

tommyip Feb 3, 2017

tommyip Feb 3, 2017

showell Feb 3, 2017

Add ability to pretty-print HTML (lib only). #3428

Add ability to pretty-print HTML (lib only). #3428

Conversation

adnrs96 commented Jan 22, 2017 • edited

smarx commented Jan 22, 2017

showell commented Jan 22, 2017

adnrs96 commented Jan 22, 2017

showell commented Jan 22, 2017

adnrs96 commented Jan 22, 2017

adnrs96 commented Jan 22, 2017

timabbott commented Jan 22, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tommyip left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adnrs96 Feb 3, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adnrs96 commented Jan 22, 2017 •

edited

adnrs96 Feb 3, 2017 •

edited