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

Mustache shouldn't alter whitespace inside partials #2

Closed
bobthecow opened this issue Jan 25, 2011 · 55 comments
Closed

Mustache shouldn't alter whitespace inside partials #2

bobthecow opened this issue Jan 25, 2011 · 55 comments

Comments

@bobthecow
Copy link
Member

Whitespace is sacred in HTML.

Mustache (per the current spec) plays fast and loose with whitespace inside partials.

This causes unexpected results:

template.mustache

<body>
    <div>
        {{> body }}
    </div>
</body>

body.mustache

<p>
    <textarea>{{ text_with_newlines }}</textarea>
</p>

context.yml

text_with_newlines: |
    This is a
    newline test

output

<body>
    <div>
        <p>
            <textarea>This is a
        newline test</textarea>
        </p>
    </div>
</body>

This is no-me-gusta. I think the "Indentation should be prepended to each line of the partial" portion of the spec should be dropped completely.

See also: this issue on Mustache.php.

@jordanthomas
Copy link

I agree. I appreciate maintaining the whitespace in partials, but in practice this doesn't work so well.

@pvande
Copy link
Contributor

pvande commented Jan 25, 2011

In practice, there are very few places in HTML where whitespace is sacred, especially when contrasted with, for example, Python or plain text (emails?). The currently specified partial behavior typically produces the same results one would achieve writing the HTML by hand. In the case of textareas and pres, as you point out, it does have negative effects.

In the given example, this can be worked around by writing either

<body>
    <div>
{{> body }}
    </div>
</body>

or

<body>
    <div>{{> body }}</div>
</body>

as your template.mustache.

I'm glad to have feedback about this, and while I'm presently content to argue for its continued inclusion, if this continues to be a point of irritation for people I will be happy to revise the spec.

@bobthecow
Copy link
Member Author

Would it make sense to use a second partials notation for whitespace prefix? Perhaps {{< foo }} vs {{> foo}} ?

@jordanthomas
Copy link

Those workarounds both work, but you're maintaining whitespace for the sake of prettiness. Breaking the tabbing seems counter-productive or at least against the point.

@pvande
Copy link
Contributor

pvande commented Jan 25, 2011

That's a potential option. Another option would be for the escaped tags to automatically replace newlines with &#10; in HTML-escaped tags (which may only solve half the problem). Research needs done.

@pvande
Copy link
Contributor

pvande commented Jan 25, 2011

@jordanthomas: In HTML, the whitespace simply there for prettiness; in any situation where whitespace is significant, the whitespace is critical. I'm not exactly sure what you mean by "breaking the tabbing", however.

@jordanthomas
Copy link

I worded that poorly, let me try again:

My point is that if you're maintaining the whitespace for prettiness and those workarounds compromise the prettiness, then what was the point in the first place?

@pvande
Copy link
Contributor

pvande commented Jan 25, 2011

Upon further reflection, it seems we've been discussing the wrong problem.

@bobthecow: Would the rendered output be acceptable?

<body>
    <div>
        <p>
            <textarea>This is a
newline test</textarea>
        </p>
    </div>
</body>

@pvande
Copy link
Contributor

pvande commented Jan 25, 2011

Please try this tree, and see if the issue is suitably resolved.

https://github.com/pvande/mustache/tree/partial-indentation-fix

@bobthecow
Copy link
Member Author

@pvande That output seems acceptable... What rule does that use? Prefix partials with whitespace, but not variables?

@pvande
Copy link
Contributor

pvande commented Jan 25, 2011

@bobthecow Partials are indented before rendering. The spec (presently) makes no ruling on when the indentation should take place, so most implementations indented the partial after rendering.

If the new behavior is suitable, v1.0.0 of the spec will contain a test proving it.

@bobthecow
Copy link
Member Author

Thanks! Off I go to update Mustache.php implementation :)

@ghost
Copy link

ghost commented Jan 3, 2013

Doesn't enforcing partial indentation mean that partial compilation can't be cached? I would actually prefer that partials weren't messed with.

@ghost
Copy link

ghost commented Jan 3, 2013

This rule is causing be all kinds of grief with my implementation. Basically, I'm caching the results of compiling partials for speed increases but I can't use my cache if it needs to be indented before render.

@groue
Copy link

groue commented Jan 3, 2013

I personally won't spend a cent of my energy on those rules until a single user opens an issue in my repo. After a few years, still nobody has complained.

@ghost
Copy link

ghost commented Jan 3, 2013

They're stopping my tests passing :(

@groue
Copy link

groue commented Jan 3, 2013

Yes. GRMustache "passes" the spec tests, simply because it absolutely ignores its whitespace "rules". It relies on its own test suite for the real job.

Actually it has the same problem as the spec suite: tests for GRMustache-specific features are intertwined with more general tests. ... I should separate them (like the support for the "else" construct, the empty closing tags, etc.)

@ghost
Copy link

ghost commented Jan 5, 2013

I've managed to have my cake and eat it! I cache compiled partials and pass the spec in it's entirety.

Incase it's of use to other implementors, I introduced a 'line' token in my parser that exists at the beginning of each line. I can then check for this token and insert the relevant amount of whitespace on render. Best of both worlds, indented partials and the benefit of having them compiled.

@groue
Copy link

groue commented Jan 8, 2013

The inconsistency and non-predictability of white-space insertion is blatant in this example with pystache (source):

template:

blah = {{> partial}};

partial.mustache:

{{#blubb}}
 "{{.}}"
{{/blubb}}

result:

blah = "blubb1" "blubb2"
;

I don't know if this behavior is required by the spec. Hint: the spec author @pvande did contribute to the indentation code of pystache.

@ghost
Copy link

ghost commented Jan 8, 2013

@groue Is that carriage return caused by a newline char after "{{.}}" or after {{/blubb}}? If it's the later, I would say that is expected behaviour.

@groue
Copy link

groue commented Jan 8, 2013

The leading \n in honored, but not the trailing one. It may be expected by the spec tests, but I can't see how a sane human who does not read the spec every day would "expect" that.

@ghost
Copy link

ghost commented Jan 8, 2013

Hmm, it's hard to see from the example, which of the following n's are preserved?

{{#blubb}}\n1
 "{{.}}"\n2
{{/blubb}}\n3

@groue
Copy link

groue commented Jan 8, 2013

Your question is exactly where the problem lies. It's impossible to "expect" the actual answer.

@ghost
Copy link

ghost commented Jan 8, 2013

From an end user perspective, I would expect only 'n3' to be honoured as it is outside of the section. Therefore, if that is what's happening, I would consider it correct.

@groue
Copy link

groue commented Jan 8, 2013

Yes, you're right, if we assume there is a third \n, which is not sure. Meanwhile, nobody answers the poor guys' question :-)

@ghost
Copy link

ghost commented Jan 8, 2013

In that case, none of them would be honoured. I think this is the sanest way to handle it, take an unordered list for example:

<ul>\n1
{{#items}}\n2
  <li>{{.}}</li>\n3
{{/items}}\n4
</ul>\n5

Here, '\n1', '\n3' and '\n5' are honoured and the others stripped resulting in what the end user would expect:

<ul>\n1
  <li>Item A</li>\n3
  <li>Item B</li>\n3
  <li>Item C</li>\n3
</li>\n5

...at least, I think (or hope) that is how the spec is defined.

@groue
Copy link

groue commented Jan 8, 2013

From the stackoverflow guy's example, it's sure that the \n3 you've juste redefined is not honored: his strings are separated by spaces, while his partial contains a CR before {{/blurb}}.

...at least, I think (or hope) that is how the spec is defined.
I've managed to have my cake and eat it!

I really don't know what cake you've eaten actually.

@ghost
Copy link

ghost commented Jan 8, 2013

What I'm saying is, the spec (or how I understand it) works as per my examples. If it's not working like that, Pystache can't be honouring the spec. I'm referring to the most recent tag.

@ghost
Copy link

ghost commented Jan 8, 2013

I initially thought the problem was the return before the semi colon.

@groue
Copy link

groue commented Jan 8, 2013

Yes, he does not want the return before the semi colon. This is his problem. But look how he gets two strings concatenated by a white space: the rendered CR can not come from the inner content of the partial section.

@ghost
Copy link

ghost commented Jan 8, 2013

He should file as bug with Pystache, that's not correct behaviour... the spec is different to that.

@groue
Copy link

groue commented Jan 8, 2013

the spec is different to that

I don't know. Pystache is spec-compliant. Thus the spec does indeed allow for this. Thus the bug, if there is one, should be filed here.

All that fuss for a useless feature, contrieved, and difficult to implement ("eat the cake", listen to you!)... I would remove that whole white-space rules out of the spec straight away.

@ghost
Copy link

ghost commented Jan 8, 2013

All I know is, partials maintaining whitespace are a useful part of the spec. If that specific example is passing the spec then there is obviously something wrong and should be fixed... it doesn't mean that the whitespace spec should be abandoned altogether.

My implementation is passing the spec in it's entirety with the ability to (pre)compile partials, pretty useful and the only implementation I've seen that can do that thanks to my 'line' token (which I thought could help other implementors, hence mentioning here). I suggest we just figure out why that edge case is getting through and fix it.

@groue
Copy link

groue commented Jan 8, 2013

useful

I'm curious actually

@ghost
Copy link

ghost commented Jan 8, 2013

Pystache is either not spec compliant or there's a loophole in the spec, as I've just run that exact example on my spec compliant implementation (spec tag v1.1.2) and it produces:

"blah =  \"blubb1\"\n \"blubb2\"\n;"

@ghost
Copy link

ghost commented Jan 8, 2013

...as I would expect.

@ghost
Copy link

ghost commented Jan 8, 2013

Why would precompiling partials not be beneficial?

@groue
Copy link

groue commented Jan 8, 2013

Who said precompiling partials was not beneficial? I can't read anything like this above.

And... do you think your own rendering is the expected one?

@ghost
Copy link

ghost commented Jan 8, 2013

Sorry, I thought you saying you were didn't think it useful... my misunderstanding.

Well, yes, my rendering is the expected as it passes the spec ;) ...if it's not expected, it's indeed a flaw in the spec. I can't see how Phstache is passing though with that implementation.

You can see my 'line' tag in my parser tests: https://github.com/thelucid/tache/blob/master/test/parser_test.rb ...it is then used at render to insert the correct amount of indentation: https://github.com/thelucid/tache/blob/master/lib/tache/template.rb#L63-L64

@groue
Copy link

groue commented Jan 8, 2013

Right. Anyway. What bugs me is twofold:

  • Pystache was written by @pvande who used to be the author of the spec. So he's supposed to be the closer of the "Mustache spirit", whatever it could means. I feel uncomfortable realizing that the code written by the guy behind the whole "Mustache white space management" can lead to situations like the one experienced in the stackoverflow question.
  • Nobody is able to say how those white space rules are, indeed, useful.

@ghost
Copy link

ghost commented Jan 8, 2013

In order:

  • If those results were in fact produced by Pystache then Pystache does not conform to the spec specifically: https://github.com/mustache/spec/blob/master/specs/sections.yml#L186-L248. This must just be an oversight in Pystache as the spec clearly states "Sections should not alter surrounding whitespace" and "Sections should not alter internal whitespace".
  • Maintaining indentation level for partials is useful for HTML output among others such as Markdown lists etc.

@pvande
Copy link
Contributor

pvande commented Jan 8, 2013

@groue: I did not write Pystache, though I did contribute some code to help it pass spec some time ago. The reported Pystache bug also has nothing to do with the automatic indentation of partial content.

Put simply, the intended whitespace behavior described by the rules cited by @thelucid are:

  • Standalone section tags should be removed entirely, including leading whitespace and trailing newline (if present).
  • Section tags should literally repeat their entire content, including any newlines, for each element of the named collection.

The rules for partials are simpler still:

  • Standalone partial tags should indent their content to the same indentation level as the partial tag.
  • All other partial tags should be literally replaced by their contents.

The provided example, rendered with Milk:

data     = blubb: [ 'blubb1', 'blubb2' ]
partials = partial: '{{#blubb}}\n "{{.}}"\n{{/blubb}}\n'
Milk.render 'blah = {{>partial}};', data, partials
# => 'blah =  "blubb1"\n "blubb2"\n;'
#     ^^^^^^^                      ^  <- From top-level template
#            ^^^^^^^^^^^^^^^^^^^^^^   <- Partial content
#            ^^^^^^^^^^^              <- First pass of section (' "{{.}}"\n')
#                       ^^^^^^^^^^^   <- Second pass of section (' "{{.}}"\n')

To get the desired result, the partial can be changed to read {{#blubb}} "{{.}}"{{/blubb}}, and the space preceding the partial tag could be omitted.

The partial indentation behavior is useful whenever the reader of the rendered output is whitespace-sensitive, as it allows you to write "context-free" partials that generate reasonable results. As an example:

{{! questions.partial }}
* Why do we care?
* How much does it cost?
* When can we buy it?

{{! needs.partial }}
* Food
  {{> questions}}
* Shelter
  * Nice house vs. Fixer
    {{> questions}}
* Freedom
  {{> questions}}

{{! philosophy.mustache }}
We must always ask three questions:

{{> questions}}

Applied to a loose interpretation of Maslow's hierarchy:

{{> needs}}

Without the partial indentation behavior, you would have unpredictable line beginnings, which would force you to a) create identical partials with different indentation levels and including the partials with no preceding whitespace, b) embed the partial content directly into the parent template, or c) give up control of whitespace. By making the calling context responsible for the partial's indentation, the partial can be more cleanly written (specifically, not arbitrarily indented), and the template reads more naturally both before and after rendering.

@ghost
Copy link

ghost commented Jan 9, 2013

@groue What he said ;)

@pvande Thanks for the concise explanation, kind of what I was trying to say but coming across as clear as mud. So the spec is correct, it's Pystache's implementation that's eating the newlines.

Liking the look of Milk, wish I knew it existed when I was writing my parser for Tache.

@groue
Copy link

groue commented Jan 9, 2013

@pvande, I thought you were dead. You, @janl and @defunkt should really not have left the repo alone like that. Now Mustache has exploded in so many forks, I'm afraid it's impossible to gather it back in a single language. An opportunity has been missed.

On the current topic, your inability to admit there is a problem in your spec as soon as implementors (pystache) or users (stack overflow guy) get confused by your "simple" ideas is astonishing, and belongs to the same "after me, the flood" behavior.

An attitude change would be welcome.

@cunger
Copy link

cunger commented Jan 9, 2013

I corrected the example on stack overflow. So the problem is not with the general newline and indentation behavior, which is fine, I think. Sorry for the confusion. Nevertheless, it would be nice to have a way to suppress newlines (like #slurp in Cheetah), in order to be able to write more human-readable templates, but this is low priority (and also not in the mustache specs, right?).

@ghost
Copy link

ghost commented Jan 9, 2013

Given the new information (that the example on StackOverflow was incorrect), I believe the spec in it's current for to be the most predictable behaviour when it comes to end users and therefore correct.

Things do get a little complicated when it comes to allowing compiled templates to be returned from view methods (as discussed with @groue) i.e. what happens to the whitespace then. In this situation I treat it in the same way as a partial which gets a little tricky as the newline for double and triple taches is then dependent on the type of object returned from the view. I get around this by including the newline in my tokens allowing me to include or exclude it from the output at render. I have yet to push my changes as it needs refactoring but seems like a good way to go to me.

In the following example, thing returns a compiled template from the view. The output will be the same as when using a partial in my implementation which I believe to be the sanest output even though it requires a little more work at render (well, only concatenating the newline for non-template return values):

The following:
  <ul>
    {{thing}}
  </ul>

Will be the same as:
  <ul>
    {{>parial_with_identical_content_to_compiled_template}}
  </ul>

@groue Which leaves only one edge case that I'm not sure how to solve and that is our {{items}} shortcut to {{#items}}{{.}}{{/items}}, you may want to also maintain the whitespace in the same way as partials and template objects, so not sure of the best approach for this.

Forgive me for a little off topic here, however it is all relevant to whitespace so thought it was the best place to mention it.

@groue
Copy link

groue commented Jan 9, 2013

Thanks @cunger for the clarification. You should maybe open an issue in the https://github.com/defunkt/pystache repo, since I don't see anything that could solve your problem in this very thread.

@thelucid I won't follow you on the white space management of {{items}} or dynamic partials, whether they are simply embedded, or processed, or whatever. Do what you want, but I bet that one day one user will be annoyed by your "useful" feature he did not request, without any way to disable it. Software that just works is better than software that tries to prove a point.

Of course, I'll be very interested by your work if one day some GRMustache user opens a white-space issue. Which I doubt.

Until then, many issues are far more interesting (meaning: interesting for end users), such as #41, #38, and the infamous array index problem (http://stackoverflow.com/questions/8567413/index-of-an-array-element-in-mustache-js, http://stackoverflow.com/questions/5021495/in-mustache-how-to-get-the-index-of-the-current-section, http://stackoverflow.com/questions/4592491/is-there-a-way-to-set-a-counter-in-a-mustache-iteration, janl/mustache.js#205, groue/GRMustache#18, groue/GRMustache#14, https://github.com/samskivert/jmustache, and I surely missed many).

@ghost
Copy link

ghost commented Jan 9, 2013

@groue Fair enough. I'm seriously tempted to head in the Handlebars direction, their helpers solve all kinds of problems. Just a shame it doesn't support save views.

@groue
Copy link

groue commented Jan 9, 2013

It's possible: GRMustache has been heavily influenced by Handlebars, while remaining a Mustache engine. Oh, and should I say Handlebars has no white space management?

@groue
Copy link

groue commented Feb 22, 2013

For those who think the topic has been so well thought it deserves this issue to be closed: I'm curious to know how you handle this user's issue: groue/GRMustache#45

@ghost
Copy link

ghost commented Feb 22, 2013

Tache solves this by using template objects (which can optionally be precompiled):

view = {
  "task-input-result" => Tache::Template.new("\/\/ Return inputs as task result\n\tprintf(\"%s\\n\", taskInput1);\nprintf(\"%s\\n\", taskInput2);\n"),
  "task-input-variables" => Tache::Template.new("\/\/ Task input variables \nchar *taskInput1 = argv[1];\nchar *taskInput2 = argv[2];\n")
}
Tache.render(template, view)

Indents will then work as expected as per groue/GRMustache#45 and only newlines are needed in the values. This is due to the concept of a 'line' token in my parser, which also allows for precompiled templates to work in the same way.

I will add more documentation to Tache for this but in the meantime see (specifically the multiline example):

@groue
Copy link

groue commented Feb 22, 2013

Thanks for the feedback, @thelucid! This is a very interesting approach indeed, to turn data into a dynamic partial, and use the spec-defined indentation. Quite smart indeed. And another validation of our approach of dynamic partials :-)

@ghost
Copy link

ghost commented Feb 22, 2013

@groue No problem. If it helps, take a look at my parser tests to see under what circumstances the 'line' tokens appear: https://github.com/thelucid/tache/blob/master/test/parser_test.rb ...and how they are used here: https://github.com/thelucid/tache/blob/master/lib/tache/template.rb

@groue
Copy link

groue commented Feb 24, 2013

Thanks @thelucid.

@groue
Copy link

groue commented Feb 24, 2013

For those interested, here is another white-space processing use case not covered by the specification: groue/GRMustache#46.

The user wants to have

{{ statement-1 }}
{{ statement-2 }}
{{ statement-3 }}

Render as:

1
3

For the following data:

{ "statement-1": "1",
  "statement-3": "3" }

Notice how the template line containing {{ statement-2 }} has not been rendered.

This issue was closed.
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

5 participants