Mustache shouldn't alter whitespace inside partials

bobthecow commented 13 years ago

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 commented 13 years ago

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

pvande commented 13 years ago

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 commented 13 years ago

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

jordanthomas commented 13 years ago

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 commented 13 years ago

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

pvande commented 13 years ago

@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 commented 13 years ago

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 commented 13 years ago

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 commented 13 years ago

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

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

bobthecow commented 13 years ago

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

pvande commented 13 years ago

@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 commented 13 years ago

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

ghost commented 11 years ago

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

ghost commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

They're stopping my tests passing :(

groue commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

@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 commented 11 years ago

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 commented 11 years ago

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

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

groue commented 11 years ago

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

ghost commented 11 years ago

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 commented 11 years ago

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

ghost commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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

groue commented 11 years ago

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 commented 11 years ago

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

groue commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

useful

I'm curious actually

ghost commented 11 years ago

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 commented 11 years ago

...as I would expect.

ghost commented 11 years ago

Why would precompiling partials not be beneficial?

groue commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

@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 commented 11 years ago

@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 commented 11 years ago

@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 commented 11 years ago

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 commented 11 years ago

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 commented 11 years ago

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, https://github.com/janl/mustache.js/pull/205, https://github.com/groue/GRMustache/issues/18, https://github.com/groue/GRMustache/issues/14, https://github.com/samskivert/jmustache, and I surely missed many).

ghost commented 11 years ago

@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 commented 11 years ago

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 commented 11 years ago

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: https://github.com/groue/GRMustache/issues/45

mustache / spec

Mustache shouldn't alter whitespace inside partials #2