Changing {% include %} to strip a trailing newline

[Tim Graham]

Shortly after template widget rendering was merged, an issue about extra newlines appearing in the rendered output was reported [0]:

For example, from django-money:

<option value="XFU" \n>UIC-Franc</option>
\n\n
<option value="USD" selected\n>US Dollar</option>
\n\n

The newlines aren't useful and they break assertions like this:

<option value="USD" selected>US Dollar</option> in form.as_p

--- end report---

The reporter suggested removing the trailing newline in the attrs.html template but Adam Johnson reported: "POSIX states that all text files should end with a newline, and some tools break when operating on text files missing the final newline. That's why git has the warning \ No newline at end of file and Github has a warning symbol for the missing newline."

I suggested that perhaps {% include %} could do .rstrip('\n') on whatever it renders.

Preston pointed out that Jinja2 does something similar:

http://jinja.pocoo.org/docs/dev/templates/#whitespace-control

• a single trailing newline is stripped if present
• other whitespace (spaces, tabs, newlines etc.) is returned unchanged

I noted that the issue of {% include %} adding extra newlines was raised in #9198 [1] but Malcolm said, "For backwards compatibility reasons we cannot change the default behaviour now (there will be templates that rely on the newline being inserted)." I was skeptical this would be a burdensome change.

Carl replied:  "It seems quite likely to me that there are templates in the wild relying on preservation of the final newline. People render preformatted text (e.g. text emails) using DTL. I probably have some text email templates in older projects myself that would break with that change.

We could add a new option to {% include %}, though." Adam also said, "I too have used DTL for text emails that would break under that behaviour. New option to include sounds good to me."

Me again: In the long run, having {% include %} remove the trailing newline seems like a more sensible default. For example, I wouldn't expect this code to have a newline inserted between it:

{% include "foo.txt" %}
{% include "bar.txt" %}
 

An option for {% include %} seems superfluous given that if you were writing

{% include "foo.txt" %}{% include "bar.txt" %}
 

now you can write the first thing which is much more intuitive.

How about a keep_trailing_newline TEMPLATES option for backwards compatibility for those who don't want to adapt their templates for the new behavior? Jinja2 has that option.

Carl replied: An engine option may be better than an option to {% include %}, though it doesn't allow us to ensure that we strip the newline in the specific case of attrs.

How we default the engine option I guess just depends on how seriously we take backwards compatibility. If we default it to strip and make people add the config to preserve the old behavior, that's not really backwards compatible. Historically (as seen in Malcolm's comment) we would choose to err on the side of actual backwards compatibility in a case like this, even if it didn't result in the ideal future behavior. But the adaptation isn't hard in this case, so I won't object if the choice is to break back-compat.

If it's not a per-include choice, of course, we have to break overall back-compat to preserve form-attr-rendering back-compat.

----

What do you think?

[0] https://github.com/django/django/pull/7769
[1] https://code.djangoproject.com/ticket/9198

[Anthony King]

I think adding the option would be the better approach, and add it in as default for new projects. 

Having used templates for text emails, I do find it unintuitive to write them because of the newline behaviour. 

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.
To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/8124d314-eb26-4465-9fc8-5b04fe2ba618%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Sam Willis]

Could this be set within the template rather than the include tag? So for example have a new tag such as {% strip_final_new_line %} that when included at the end of a template immediately before the final new line would instruct it to be striped.

This stops the user from having to remember to use a special option on the include tag for each use of a template that requires it - it also work on a normal (not included) template render as well, if that is wanted.

An alternative tag could be {% endtemplate %} that can be placed anywhere in the template, forcing the end of rendering?

[Tim Graham]

Anthony, I assume you're referring to the keep_trailing_newline  option? If we don't enable it for existing projects, then forms will render with extra newlines. I think it's better to have projects opt-out of the behavior if they're unable to audit their non-HTML templates due to time constraints. Only enabling it for new projects means that the tests for projects like django-money will break until the option is enabled. I consider the keep_trailing_newline option a temporary backwards compatibility measure and would like to eventually remove it.

Sam, re: "Could this be set within the template rather than the include tag?"
I'm not an expert in the inner workings of the Django template language, but based on how template tags usually work, that doesn't feel feasible. To be clear, I'm not proposing to add an option to the {% include %} tag. That seems like the wrong approach to me since if {% include %} already strips the trailing newline of its output, then you can control newlines by how you structure the {% include %} tag. This seems far more intuitive and readable.

I also considered a technique we've used to change template tag behaviors previously: {% load include from future %} however, I feel that would be really disruptive to force all projects to go through that deprecation.

[Anthony King]

Tim, yes I meant the keep_trailing_newline option.

Digging through the code (as I mentioned on IRC), it's possible to have the form rendering to use different options.

Here is a proof of concept [0], which forces removal of trailing whitespace in forms, and adds it as an option for settings.

I'm unsure of the behaviour in Jinja. The naming seems wrong for it to be for {% include %} only, however this is just a proof of concept.

If you're happy with it, I'll contribute it to the currently open PR.

[0] https://gist.github.com/anonymous/098c076f626466dc3154e3a6da02d996

--

You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.
To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.
Visit this group at https://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/de92a16a-25c7-439a-95a1-3a2b43a6c1ac%40googlegroups.com.

[Aymeric Augustin]

If I understand correctly, we have to choose between:

1. breaking backwards compatibility for {% include %}

2. breaking backwards compatibility for widgets HTML

3. having a handful of single-line, non-newline-terminated files

I don’t think option 1 is reasonable. Whitespace control in templates is an exercise in frustration. In my experience more flexible tools such as {%-, {%+, -%}, and +%} in Jinja2 increase the frustration. I don’t think Django should get itself into this mess just for fixing the problem discussed here.

Option 2 seems acceptable to me. This is the "do nothing” option. I don’t think the exact HTML string rendered by widgets is considered a public API. We made changes to that output in the past, for example when we switched to HTML5.

Option 3 also seems reasonable to me. Files could end with {# no newline, see issue #xxx #} and no newline. A test could validate that the rendered output doesn’t contain a newline. This would be enough to catch accidental regressions caused by editors automatically adding the newline.

The quote from the POSIX standard says “should”, not “must”. If we interpret it as if it was written in capitals in a RFC, this means we can make an exception if we have a good reason.

(At this point surely someone will suggest that we can write an editorconfig to specify that these files mustn’t end with a newline. Whatever works.)

I don’t have a strong preference for 2 or 3, however, I have a strong distaste for anything more complicated.

Best regards,

-- 

Aymeric.

--
You received this message because you are subscribed to the Google Groups "Django developers (Contributions to Django itself)" group.

To unsubscribe from this group and stop receiving emails from it, send an email to django-develop...@googlegroups.com.
To post to this group, send email to django-d...@googlegroups.com.

Visit this group at https://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/8124d314-eb26-4465-9fc8-5b04fe2ba618%40googlegroups.com.

[Tim Graham]

Aymeric, we have a difference of opinion. I feel that if {% include %} removed the trailing newline, it would result in far more intuitive whitespace control (which may be needed in plain text email templates, for example). I think the change has merits outside the context of this issue. For example, I checked view-source on a djangoproject.com page that uses a {% for ... %}{% include %}{% endfor %} construct and noticed that it's much shorter without all those extra blank lines. I'm not advocating for additional controls to craft well formatted HTML but I think there's some advantages to not having blank lines everywhere. (I don't like the keep_trailing_newline option either, I'd rather call it a bugfix and make a backwards-incompatible change but my projects are unaffected as far as I checked.)

There are many questions on Stackoverflow about how to suppress newlines from {% include %} and even a django-include-strip-tag third-party app that offers the feature.

In the context of multiple template engines, I think it would be useful if DTL templates could be written in a similar style to Jinja2.

I'd like to know from Carl, Adam, and others, how much effort would be required to adapt the templates in your project for the new behavior. I imagine a script could be written to add newlines after all {% include %} in all plain text templates, for example.

It won't be the end of the world if we can't get consensus to change the behavior -- I'm okay with removing the trailing newlines in the source files as needed, although it will add some inconvenience.

[Adam Johnson]

Whitespace control in templates is an exercise in frustration. In my experience more flexible tools such as {%-, {%+, -%}, and +%} in Jinja2 increase the frustration.

I really enjoy the {%- etc. operators in Jinja 2 in the context of Ansible, there are often cases when templating obscure configuration files that they come in useful.

 

I'd like to know from Carl, Adam, and others, how much effort would be required to adapt the templates in your project for the new behavior. I imagine a script could be written to add newlines after all {% include %} in all plain text templates, for example.

You're right, it's not too hard to fix for the text email template usecase. But the backwards compatible option is probably necessary for some more advanced uses of templates out there, e.g. templating whitespace sensitive file formats.

To unsubscribe from this group and stop receiving emails from it, send an email to django-developers+unsubscribe@googlegroups.com.
To post to this group, send email to django-developers@googlegroups.com.

Visit this group at https://groups.google.com/group/django-developers.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/980cf18c-20e2-411a-9536-acea1e4d3556%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Adam

[Curtis Maloney]

On 05/01/17 10:09, Adam Johnson wrote:
> Whitespace control in templates is an exercise in frustration. In my
> experience more flexible tools such as {%-, {%+, -%}, and +%} in
> Jinja2 increase the frustration.
>
>
> I really enjoy the {%- etc. operators in Jinja 2 in the context of
> Ansible, there are often cases when templating obscure configuration
> files that they come in useful.

I believe SmileyChris had a patch to add this to DTL...

--
Curtis

[Carl Meyer]

On 01/04/2017 02:53 PM, Tim Graham wrote:
> I'd like to know from Carl, Adam, and others, how much effort would be
> required to adapt the templates in your project for the new behavior. I
> imagine a script could be written to add newlines after all {% include
> %} in all plain text templates, for example.

Not much, I'd expect. Plain text templates aren't the common case, and
many of them don't use {% include %} anyway. And as you say, it is a
scriptable change if necessary.

That said, IMO the perceived cost of backwards-incompatible changes at
upgrade time scales with the number of changes required, even when
individually they don't require much work to fix. In theory, we promise
backwards-compatibility, and I don't think "call it a bugfix" can be a
universal escape hatch; it's clear that the current behavior has been
known and accepted for years. If we change this, it's a small but
incompatible change in the known and intended behavior, it's not fixing
a bug. But we've bitten the bullet and made far larger
backwards-incompatible changes in the past, so...

There is also the possibility that someone is relying on this behavior
in a more intentional way than I've ever done myself; e.g. has an
include file with multiple trailing newlines or other trailing
whitespace, specifically because it's desired in multiple different
places (in a preformatted text context). This (admittedly hypothetical)
case isn't quite as trivial to work around, if you propose to strip all
trailing whitespace. Or do you propose to strip only exactly one
trailing newline character?

In the end, I'm happy to abstain from this decision; I wouldn't really
be upset by any of the three options Aymeric listed.

Carl

[Tim Graham]

About "the backwards compatible option is probably necessary for some more advanced uses of templates out there, e.g. templating whitespace sensitive file formats." -- I'm not following why a similar find/replace approach wouldn't be sufficient to adapt those templates?

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/980cf18c-20e2-411a-9536-acea1e4d3556%40googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

--

Adam

[Adam Johnson]

Actually you're right it's probably the same

> Or do you propose to strip only exactly one
trailing newline character?

I thought the proposal was only for exactly one

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/d3390e72-7dc4-44bb-bac1-3546ad894cf1%40googlegroups.com.

[Aymeric Augustin]

Hello,

I’m not going to veto whitespace control in Django templates. Mostly I’m uncomfortable with rushing this feature to fix the much narrower problem discussed here.

If we want to provide whitespace control in templates, we should review use cases, other implementations, and figure out something reasonable.

I hope this clarifies my position,

-- 

Aymeric.

To view this discussion on the web visit https://groups.google.com/d/msgid/django-developers/980cf18c-20e2-411a-9536-acea1e4d3556%40googlegroups.com.

[charettes]

Hey Tim,

I just wanted to mention that another approach for a transition plan could be
to use {% load include from future %} like we did with the {% url %} tag.

That doesn't answer the question of whether or not we should make the change
but I think it offers a better upgrade path than a global template option or
a keep_trailing_newline kwarg.

Simon

[Tim Graham]

Hi Simon, I mentioned that idea offhand in an earlier mail, however, I feel that it would be really disruptive to force all projects to go through that deprecation and require adding {% load include from future %} to every template that uses the tag to silence warnings (and then remove those lines sometime after the deprecation ends). (More disruptive than just making a backwards-incompatible change that's likely to affect a (hopefully small) subset of users.) I guess I'm open to it if others think that's the way to go though.

[Alasdair Nicol]

Would adding the future include tag to the builtins in the template OPTIONS work? If it did, then you'd only have to make the change to the TEMPLATES setting instead of every template.

    OPTIONS={

        'builtins': ['django.template.future_include'],  # put new include in it's own module so that we don't add anything else in future.

    }

I'm not familiar with that code, so I don't know whether that would override the old include tag. 

Cheers,

Alasdair

[Tim Graham]

I'd like to merge the current patch that removes the trailing newline in the attrs.html template, then revisit the issue of {% include %} stripping the trailing newline for the next version of Django so we don't need to rush a decision about that.

[Florian Apolloner]

On Friday, January 6, 2017 at 3:13:27 PM UTC+1, Tim Graham wrote:

I'd like to merge the current patch that removes the trailing newline in the attrs.html template, then revisit the issue of {% include %} stripping the trailing newline for the next version of Django so we don't need to rush a decision about that.

Sounds good!