Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Inserting final newlines isn’t safe #56

Closed
thibaudcolas opened this issue Feb 12, 2022 · 4 comments
Closed

Inserting final newlines isn’t safe #56

thibaudcolas opened this issue Feb 12, 2022 · 4 comments
Assignees

Comments

@thibaudcolas
Copy link

thibaudcolas commented Feb 12, 2022

I’ve been trying out djhtml on a project and our visual regression testing showed there were differences before-after the reformatting. Upon further inspection this seems to be due to djhtml inserting final newlines in some occurences.

Looking at the code, this seems to be done on purpose, but there are two issues:

  • Whitespace is often sensitive in HTML – so inserting whitespace where there wasn’t any will change how the page renders.
  • The "insertion of final newline" logic isn’t consistent – djhtml will only add the newline when it made other changes to the template. It won’t add the newline if the template’s formatting is already good.

Here is a practical example where this happens. With an icon.html template:

<svg>
<use href="#my-icon"></use>
</svg>

And a button.html:

<button>{% include "icon.html" }My button</button>

Without a final newline in icon.html, this renders as:

<button<svg>
<use href="#my-icon"></use>
</svg>My button</button>

With a final newline (which djhtml inserts) in icon.html, this becomes:

<button<svg>
    <use href="#my-icon"></use>
</svg>
My button</button>

This will cause additional space between the icon and the text. I’ve made a small demo to help illustrate.


As a resolution to this, my preference would be for djhtml to completely stop inserting final newlines. There are a lot of tools that do this already for people who want to, and it’s not something that can be applied to existing HTML templates with full certainty as the behavior depends on how the partials are used.

@JaapJoris
Copy link
Member

Hi, welcome, and thank your very much for reporting this issue!

First of all, all text files should end with a newline character. Therefore, I think the current behavior should stay the default. Editing your SVG file in VSCode, for example, will also append a final newline, as do Vim, Emacs, and most other editors. According to the Robustness Principle, it'd best if your web application does not depend on the absence or presence of a final newline in the SVG source code.

However, you are completely right about two things:

  1. DjHTML is inconsistent; it will not add a newline if the source file is otherwise correctly indented.
  2. It would be nice if the option to omit the final newline exists.

I will be happy to accept a PR that fixes this. Until then, you could have a look at this StackOverflow discussion for various ways to get rid of the added visual space between inline elements.

@JaapJoris JaapJoris added the help wanted Extra attention is needed label Feb 12, 2022
@thibaudcolas
Copy link
Author

thibaudcolas commented Feb 14, 2022

Hey @JaapJoris, thank you for looking into this. I agree text files should end with a newline character, and most projects I work on do enforce this in some form like you suggest. However I still think this isn’t something djhtml should enforce.

My interpretation of the robustness principle’s “be conservative in what you do, be liberal in what you accept from others” in the case of djhtml is that it should accept files without a final newline (it does), and be conservative about doing the kind of change that’s happening here (it’s not), as adding newlines in whitespace-sensitive text isn’t safe. The README of djhtml even states:

DjHTML is an indenter and not a formatter: it will only add/remove whitespace at the beginning of lines. It will not insert newlines or other characters.

Adding a final newline isn’t indenting, and it clearly is "inserting newlines", so the current behavior is at odds with the goals stated in the README. I don’t think those goals should be compromised, as it’s completely unsafe to insert newlines in-between tags in HTML templates.


Re other solutions and your StackOverflow link, the most popular answer’s first pattern is:

<p>
    <span>Foo</span><span>Bar</span>
</p>

This is exactly what we’re doing right now, and what djhtml breaks (since we’re using a separate template for the <span>Foo</span>). All of the other patterns suggested in that answer wouldn’t make any difference when using djhtml – as again the insertion of the newline happens within a template inserted with {% include %}. We could in theory refactor our icon template to end in <!--, and then update each and every usage of that partial to add --> afterwards, thereby discarding the extra newline, but that’s really working against the readability of the templates.

Solutions based on CSS are also not realistic – they assume you have complete control over where the HTML is used, which isn’t the case for all projects, particularly for a highly reusable component like the icon I mention which should assume as little as possible about where it’s used.

@JaapJoris
Copy link
Member

Thank you @thibaudcolas for your thoughtful argument. You have successfully convinced me.

As the README states, the goal is to correctly indent already well-structured templates, not to fix broken ones. A text file with a missing final newline is broken, but it should not be DjHTML's job to fix that. When I created DjHTML I thought appending a final newline was a handy extra feature, but you made me realize it is actually a bug.

I will remove this feature as you requested and release a new version of DjHTML without it.

@JaapJoris JaapJoris removed the help wanted Extra attention is needed label Feb 14, 2022
@JaapJoris JaapJoris self-assigned this Feb 14, 2022
@thibaudcolas
Copy link
Author

Yay! Recommending the end-of-file-fixer for this makes a lot of sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants