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

[BUG] [Formatter] endtag and closing tag on same line are adding whitespace #167

Closed
3 tasks done
gone opened this issue Jan 5, 2022 · 9 comments
Closed
3 tasks done
Assignees
Labels
🦠 bug Something isn't working 🧽 formatter
Projects

Comments

@gone
Copy link

gone commented Jan 5, 2022

System Info

  • OS: Ubuntu 21.10
  • Python Version: 3.9.7
  • djLint Version: 0.7.1
  • template language: Django

Issue

Sometimes I'm seeing the formatter incorrect indent when two closing tags are right next to each other as shown in this diff:

{% extends "base.html" %}
-
 {% load i18n %}
 {% load account %}
-
-{% block head_title %}{% trans "Sign In" %}{% endblock %}
-
-{% block content %}
-
-<h1>{% trans "Sign In" %}</h1>
-
-<p>{% blocktrans %}If you have not created an account yet, then please
-<a href="{{ signup_url }}">sign up</a> first.{% endblocktrans %}</p>
-
-<form class="login" method="POST" action="{% url 'account_login' %}">
-  {% csrf_token %}
-  {{ form.as_p }}
-  {% if redirect_field_value %}
-  <input type="hidden" name="{{ redirect_field_name }}" value="{{ redirect_field_value }}" />
-  {% endif %}
-  <a class="button secondaryAction" href="{% url 'account_reset_password' %}">{% trans "Forgot Password?" %}</a>
-  <button class="primaryAction" type="submit">{% trans "Sign In" %}</button>
-</form>
-
+{% block head_title %}
+    {% trans "Sign In" %}
 {% endblock %}
+{% block content %}
+    <h1>{% trans "Sign In" %}</h1>
+    <p>
+        {% blocktrans %}If you have not created an account yet, then please
+            <a href="{{ signup_url }}">sign up</a> first.{% endblocktrans %}
+        </p>
+        <form class="login" method="POST" action="{% url 'account_login' %}">
+            {% csrf_token %}
+            {{ form.as_p }}
+            {% if redirect_field_value %}
+                <input type="hidden"
+                       name="{{ redirect_field_name }}"
+                       value="{{ redirect_field_value }}"/>
+            {% endif %}
+            <a class="button secondaryAction"
+               href="{% url 'account_reset_password' %}">{% trans "Forgot Password?" %}</a>
+            <button class="primaryAction" type="submit">{% trans "Sign In" %}</button>
+        </form>
+    {% endblock %}

You can see the </p> and everything after, right after the endblocktrans, should be dedented one level

How To Reproduce

I see it every time I run the formatter against that specific template

@gone gone added 🦠 bug Something isn't working 🧽 formatter labels Jan 5, 2022
@github-actions github-actions bot added this to Needs triage in bugs Jan 5, 2022
@christopherpickering
Copy link
Contributor

Thanks for reporting! The issue is the blocktranslate.. they are kinda a pain as you can add a trimmed variable to them and then django will strip whitespace...meaning we can format it.

But when we don't put trimmed in, we should not format it. The hard part is know which close tag {% endblocktrans %} belongs to the {% blocktrans %} or {% blocktrans trimmed %}.

I suppose the options for fixing djlint are - find a way to match start tags to close tags, or just ignore any blocktrans.

@christopherpickering
Copy link
Contributor

Probably the best thing to do it just ignore {% blocktrans %} from the formatter and let users add it to their config file as a custom block if they want them formatted.

@jayvdb @matthiask do you guys have an opinion on this?

@christopherpickering
Copy link
Contributor

I thought a bit more about this, I think there are other cases where this is a problem, outside of the translate, as well.

For example, unmatched html tags inside of template tags:

{% if %}
<div>
{% endif %}
{% if %}
</div>
{% endif %}

will format like this:

{% if %}
  <div>
    {% endif %}
    {% if %}
   </div>
{% endif %}

which is also wrong.

Its probably just time to rewrite the indenter. Right now it is traversing the code linearly, and looking for tags to indent/unindent. We should probably use similar log to linter rule H025 and find tag pairs to indent instead.

@christopherpickering
Copy link
Contributor

I opened #168 for folks using the trimmed tag. I'll push a fix for your problem shortly. It will basically exclude all {% blocktrans(late) %} from the formatter. Ultimately I'll rewrite the indenter so that we can format {% blocktrans(late) trimmed %} blocks.

christopherpickering pushed a commit that referenced this issue Jan 7, 2022
@dyve
Copy link

dyve commented Jan 8, 2022

We sometimes use

{% if is_active %}
    <li class="active">
{% else %}
    <li>
{% endif %}
   some content
</li>

At one point you just have to admit defeat. Even with a rewritten parser, how would you indent the above so that both the {% if %} and the li tags line up.

If we accept the above as "code smell", we could say the better way to do this is to separate hierarchies of HTML and template tags when possible.

<li{% if is_active %} class="active"{% endif %}>
   some content
</li>

@gone
Copy link
Author

gone commented Jan 11, 2022

I agree that the trimmed/non trimmed case is special enough to not be included in the default.

For the li case - I haven't thought about it too hard to see where this breaks down, but if the rule is "indent one layer on li, and dedent on layer on else/endif" that would come pretty close, wouldn't it? The "some content" then would be flat against the left side, but the meaningful pieces would be closer to where you expect.

I wonder if you could have two different parse trees to guess indentation, one for template tags and one for html. The html tree would be pretty straight forward, and the templatetag would be a mess of special cases.

@christopherpickering
Copy link
Contributor

Thanks! This should be fixed in the latest commits, I'll publish a release soon.

bugs automation moved this from Needs triage to Closed Jan 14, 2022
@gone
Copy link
Author

gone commented Feb 7, 2022

Things worked great for me on the newest version. Thanks!

@christopherpickering
Copy link
Contributor

Nice, thanks for testing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🦠 bug Something isn't working 🧽 formatter
Projects
bugs
  
Closed
Development

No branches or pull requests

3 participants