Optional inheritance spec

[jgonggrijp]

This is a continuation of #75, updating to the latest master, marking the spec as optional and adding some polishing on top, such as making the terminology a bit more consistent and making the overview description more precise and explicit. Prior discussion that led to this PR started at #75 (review) and extends all the way to the bottom of #75. The commit list below gives the full account of what I've done.

@gasche and I believe that inclusion of the spec does not require a major version bump, if it is marked optional as I've done here.

Since the original PR reflected existing practice, I have not permitted myself to make any syntactic or semantic additions (or even changes) to the specs. As a consequence, I have not addressed some known open ends:

• The GRMustache test suite contains some additional specifications which are not included here. As far as I understood from the older discussion in Add template inheritance spec #75, not all of those specifications were shared with other existing implementations.
• The spec currently leaves unspecified whether argument blocks are evaluated in the context of the inheriting template or the parent template. Mustache.php, @gasche and I agree that it should be the latter.
• The spec leaves unspecified what exactly should happen with the indentation of blocks.

I believe that, despite these "gaps", it would be worthwhile to merge the new spec as-is, if only because it has been lying around for so long. The gaps could be addressed in separate, future PRs and discussions.

cc @jazzdan @bobthecow @groue

[gasche]

Thanks for this work! I haven't done a full review of the new specification for now, but I agree that we should merge this (as an optional feature), to reflect the current status of implementations that have converged on this specification. (Keeping this outside the specification is a disservice to implementators, who don't have a convenient way to discuss and test their implementations, and to users, who have a harder time assessing how reliable the feature is).

Out of the three "optional improvements for later" points highlighted by @jgonggrijp, I think that the most important is the second one:

The spec currently leaves unspecified whether argument blocks are evaluated in the context of the inheriting template or the parent template. Mustache.php, @gasche and I agree that it should be the latter.

Currently the spec does not specifies this, and it would be nice if we could get reassurance that existing implementations agree on the behavior, or discuss which choice to make. I can support the idea of keeping this question for later, but in fact I would even prefer if we could resolve it in the specification right now, as it is currently incomplete.

[jgonggrijp]

I agree with you, @gasche. I would like to make some additions so that the scope of blocks is specified and so that we have something concrete to discuss. Before I do that, though, I think I should implement the extension in my own Mustache engine, in order to ensure that I understand 100% what I'm doing. That means it will take a few more weeks, since I'm doing all of this in my spare time. I guess that after eight years of inactivity, a few more weeks don't matter much, though.

[gasche]

I read the proposed description. I have mixed feelings about the current proposal: it is more precise and I think better for implementation, but also longer and, I think, less clear to people who don't already know about the feature.

"Parent tags" (I like the terminology choices, thanks!) have a disadvantage compared to other Mustache features, which is that they are not documented in the (rather good) historic user-oriented documentation. Currently people can learn about it from the description of #75, and in the long-term we may hope that user-oriented documentation will be extended to cover this feature, but in the meantime I think that it is likely that "reading the spec" will remain the main way to learn about the feature for some people (or at least to get more knowledge than a basic example), so there is more pressure to make it readable than other parts of the spec. In particular, we might consider having examples in the main body of the text.

I realize that I'm moving the goalposts here, coming up with preferences that we hadn't mentioned before -- actually I didn't think about this before making a new read of the proposed specification. I'm thinking of trying to help by sending @jgonggrijp some rewrite suggests (as a PR to his own PR); in the meantime, one simple recommendation / change proposal would be to start the description with a few paragraph that summarize the feature (this is not so far from the original text of #75), aiming for clarity for people unfamiliar with it, and then in a second part have the full precise details.

[jgonggrijp]

@gasche I agree that the lack/outdatedness of user-oriented documentation is a problem. How about just fixing the manpage, though? I already wanted to do that, anyway.

[gasche]

I don't know how/where the mustache manpage is hosted and who has access to it. But yes that sounds like a good idea in principle. (Still I would be in favor of making the specification more reader-oriented, but I agree that not having it as the main entry point for documentation would be better.)

[jgonggrijp]

The manpage is hosted from https://github.com/mustache/mustache.github.com, although the source is probably from the original Ruby implementation at https://github.com/mustache/mustache. Neither repository seems entirely dead, so I don't think it would necessarily be difficult to get a PR merged. I plan to submit a PR over there next, and I propose to postpone a decision about the overview text in the current PR until after I have done that.

While I strongly believe that the status quo is undesirable and that users should just be able to get accurate, up-to-date information from the manpage instead, I'm not opposed to making the spec overview more reader-oriented. You are welcome to submit a PR to my branch, @gasche, so that we can perfect the text together. You might want to wait with that until I've submitted my suggestions to the manpage, but if you want to do it sooner, be my guest.

[bobthecow]

A few thoughts on wording, but in general it feels solid.

[bobthecow]

Because of the collision with "argument" and "parameter" in programming languages, it feels like picking an entirely different thing to call these would be preferable. Can we use "block name" rather than "parameter or argument" to avoid the ambiguity around first/last override winning?

If so, should we not call them parametric partials / parameterized partials / etc at all?

[jgonggrijp]

What do you mean by "collision"? Parents with blocks behave pretty much exactly like functions with (default) arguments. See also my discussion with @gasche at the bottom of #75.

[bobthecow]

I'm not sure this paragraph needs to exist to make the spec work.

[jgonggrijp]

I feel that it clarifies why the file is called "inheritance" at all, and why the word "substitution" is used in some of the specs. After my discussion with @gasche, I feel that ~parametric-partial.yml would actually be more straightforward for new people to understand where the name is coming from. If we remove this paragraph, I feel that the word "substitution" should also be removed from all specs.

[bobthecow]

I'm on board with marking this as an optional feature and only doing a minor version bump. That feels like a good compromise.

[Danappelxx]

Thank you for moving this along! Marking this as optional does allow us to sidestep a major version bump, which is good.

I believe that, despite these "gaps", it would be worthwhile to merge the new spec as-is, if only because it has been lying around for so long. The gaps could be addressed in separate, future PRs and discussions.

Totally agree.

Once comments are resolved I'll go ahead and merge :)

[sayrer]

I'll fix up https://github.com/twitter/hogan.js for this if need be.

[jgonggrijp]

Update @gasche @bobthecow @Danappelxx I have pushed my first "go" at updating the manpage here:

jgonggrijp/mustache@dded806

I didn't submit a PR yet because I'm still struggling with getting rake man to work. I'm fairly content with the text changes, however, so you can already comment on it, suggest changes, etcetera. This might change the perspective a bit on some of the things we've been discussing in the current PR.

[jgonggrijp]

Another update: I managed to get rake man to sort-of work and submitted a PR. See reference directly above this comment.

[Danappelxx]

Merging! Thank you to all who contributed to the spec and discussions. (v1.2.0)

[jgonggrijp]

@Danappelxx Thank you for merging. To be honest, this took me by surprise, because I thought the discussion wasn't completely resolved yet.

I completed my own implementation of the inheritance spec just yesterday and I'm still looking into some possible additions to the spec. No problem though, I can just submit a new PR when I'm done with that.

[Danappelxx]

Sorry! Figured 7 years was enough time for bike shedding ;)

Definitely free to make new pr's, which should go by much faster now that the initial spec is in.

[jgonggrijp]

@gasche FYI, I'm working on two additions: one to specify scope resolution in blocks and one to be more precise on whitespace handling. I'll discuss the latter in an issue ticket before submitting a PR. The former seems less controversial (and more urgent), so I'll likely submit a PR directly when I'm done with it.

[gasche]

Thanks! I apologize for not being reactive on this issue, I haven't had a lot of time to work on mustache recently, but I'm very happy with your efforts and @Danappelxx pragmatic moving-forward choices. Please don't put too much pressure on yourself, the spec situation has been rather-bad for years, and imperfect improvements are already huge.

[gasche]

(For the record, I have also implemented template inheritance in ocaml-mustache sometime last year, and the evaluation-context-for-parameter-blocks issue is the only one I identified as missing in the then-proposed spec.)

[bobthecow]

Thanks for merging! Mustache.php's test suite has been updated to v1.2.0 and pulls in the inheritance spec.

[jgonggrijp]

@gasche @bobthecow I'm now hosting my own copy of the updated manpage, so there's a link that we can share from now on when we want to provide up-to-date documentation to the template language itself. See mustache/mustache#266 (comment) for further details.

[jgonggrijp]

Update to the official website being outdated: it is much less outdated now! The mustache(5) manpage is now current with the latest version of the spec.