search

mustache / spec

The Mustache spec.
MIT License
372 stars 71 forks source link

Optional inheritance spec #125

Closed jgonggrijp closed 3 years ago

jgonggrijp commented 3 years ago

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 https://github.com/mustache/spec/pull/75#pullrequestreview-607653620 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:

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

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

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

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 https://github.com/mustache/spec/pull/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 commented 3 years ago

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

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

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

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

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

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

jgonggrijp commented 3 years ago

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

https://github.com/jgonggrijp/mustache/commit/dded80614792339fc2d044d5985c4df66d2a57e7

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

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

Danappelxx commented 3 years ago

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

jgonggrijp commented 3 years ago

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

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

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

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

(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 commented 3 years ago

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

jgonggrijp commented 2 years ago

@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 https://github.com/mustache/mustache/pull/266#issuecomment-1014986964 for further details.

jgonggrijp commented 1 year ago

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.