PATCH BOMB: Scala 2.11 cross compile, separate data Binding concerns from Context, parsing fixes

[timcharper]

This is a big one!

I've taken care to make the tests pass with each patch, so each incremental change can be independently evaluated.

I've also simplified the ContextFactory, quite drastically. I think the different factory concerns are a data-binding concern, and should be abstracted at that level. I haven't implement caching quite the way it was implemented before, but it SHOULD be thread-safe as there is not any use of the Scala 2.10 experimental reflection library.

I've done a big push to remove data binding concerns from Context, Visitor, and the Helper routines. It's not 100% there, but this restructuring is a big step Data binding concerns are completely removed from Context, Visitor, and Helper routines; existing external API behavior has been preserved as specified by the tests.~~

Why all this work? My goal is to use handlebars in a much more type-safe way. I'd like to plug in a play-json binding and render my handlebars partials using JSON AST data. With data-binding tightly coupled to view, helper, visitor, this was impossible. Now, it's within reach.

Please consider accepting these changes. I'm happy to continue collaborating to push it over the line so we can release handlebars-play-json.

[timcharper]

As seen in https://github.com/SpinGo/handlebars.scala/commit/297ea900f72341aeb7507036d5fc37c5c02a4d7e, support for 2.9.x has been dropped. This was necessary as scala-test 2.1.6 (2.11.x compatible) only cross-compiles to 2.9. I think supporting 3 versions of scala will be very difficult as most library authors are eager to take advantage of new scala features.

If it's important to keep 2.9.x compatibility, we could compile to that level. Actually running the tests, however, could be tricky as it will require getting the tests to compile and work the same between two different library versions.

[timcharper]

@mwunsch @chrhicks I've updated the pull-request with a few minor changes, including the requested change to .travis.yml; The build is now passing.

[timcharper]

I don't know where a different integrations will be, but the following reflection-free (except for pattern matching usage) json data-binding is now possible:

https://github.com/SpinGo/handlebars-play-json/tree/master

Main.scala:

package fun

import com.gilt.handlebars.Handlebars
import com.spingo.handlebars.PlayJsonBinding._
import play.api.libs.json._

object Main extends App {
  val json = Json.parse("""
    {
      "lucky": [1,2,3,5,8],
      "name": { "first":  "Tim"},
      "favorites": {
        "color": "blue",
        "language": "scala"
      }
    }""")

  val template = """
My name is {{name.first}}.

These are my favorite #'s:
{{#lucky}}
- {{this}}{{/lucky}}

These are my favorite things:
{{#each favorites}}
- {{@key}}: {{this}}{{/each}}
"""
  val hb = Handlebars(template)

  println(hb(json))
}

[timcharper]

I've made a few fixes to the patch allow different binding strategies via implicit BindingFactory. I've pushed the Binding concept even further through, and it cleaned up some otherwise wonky edges and allowed preservation of type information for mustache AST-provided primitives.

[timcharper]

@mwunsch @chrhicks I've aded another change to the grammar such that it will properly handle escaped handlebars

[chrhicks]

Tim I just looked at all the code you change. Overall I think this is amazing. I left some random notes for you.

I'm kind of lukewarm on Bindings but I totally see the potential of having them. I'm going to check out handlebars-play-json as well. Do you have a project where you're using it that I can check out? Im curious how you're using it in play (we use play heavily at Gilt too).

Give me a day or two to mull this over? I want to look at how we're using handlebars.scala at gilt and see if I can't come up with any more comments.

[timcharper]

Great, thank you.

I can understand why you would feel lukewarm on bindings; however, here are some reasons why I feel it's important to strongly consider something like it:

• Without bindings, the logic for how to access dynamic data (evaluate truthiness, grab a collection, etc.) is spread throughout the library (context, helpers, visitor, helperOptions, etc.). This conaissance (born together) of algorithm leads to less manageable and more-difficult-to-understand code.
• It enables a clean way to for the user to control truthy values (and more closely match JavaScript behavior if that's a goal worth striving for).
• It allows closer emulation of handlebarsjs
• It allows for more type-safe interaction with data (binding JsValue allows me to restrict, in an at-compile-time fashion, what type of data I can use in handlebars. I don't think the value of this can be overstated)
• While I think the reflective binding is cool, it is a leaky abstraction and can lead to surprising behavior (especially with implicit methods). My preference is to bind to an AST-like data-structure, where the type of each node is modeled and restricted. With data-bindings, we can bring our own and both be happy.

Our use-case is currently behind closed doors, but it's growing. We have a case in which we are sharing handlebars templates in the browser, and it's important that we get them to behave very similarly. Also, having bindings allows me the flexibility ability to "fail loud and proud" if a handlebars template is not in alignment with the provided data. This is very important and helps us to catch bugs earlier; we'd prefer the system to fail and do nothing than do the wrong thing.

So, it's definitely useful for us! And I think the separation of concerns makes the code easier to understand. I think we could solve caching pretty well, with better reuse than before, via a light-weight LRU caching scheme. This library looks perfect: https://code.google.com/p/concurrentlinkedhashmap/

I'll do another pass on naming per your comments; I've been so hard at work on this the past couple of days I haven't done a good clean-up round. I was thinking it would be good to add tests to the DynamicBinding itself.

[timcharper]

@chrhicks what do you think of these patches? Is it getting there? You feeling it? Or still no?

I think the LRU cache implementation will lead to much better re-use of values, without the needed to pass around a cache object or worry about too large of memory allocated. The specs run 15-20% faster with it enabled.

[chrhicks]

I think this is good.

If you could do me one more favor and document Binding.scala (ScalaDoc style) so that we can remember the expectations of a Binding object in the future that would be great.

Once thats done I'm ready to merge this. :)

Sorry for the slow responses, been a little busy lately.

[timcharper]

Great,

How about I start with the aforementioned specs, and then I'll add a section to the README.

On Sun, Jun 8, 2014 at 7:58 PM, Chris Hicks notifications@github.com wrote:

I think this is good.

If you could do me one more favor and document Binding.scala so that we can remember the expectations of a Binding object in the future that would be great.

Once thats done I'm ready to merge this. :)

Sorry for the slow responses, been a little busy lately.

— Reply to this email directly or view it on GitHub https://github.com/mwunsch/handlebars.scala/pull/39#issuecomment-45455608 .

[chrhicks]

I meant to suggest writing specs for it as well but I forgot!

That would be great.

[timcharper]

@chrhicks please review this latest patch; I've simplified the Binding interface significantly by reducing unnecessary levels of undefined / void. I've made naming more consistent, added tests and also documented the binding interface in the README.

I'm sure the docs aren't perfect and will require a refinement or two, but I feel like the Binding interface is a ton more clear.

[chrhicks]

@timcharper I really like how this is shaping up with the refactor you just did!

I left a few minor (I think) comments!

[timcharper]

Was able to resurrect the commit notes by restoring the original commit. I'll post the changes individually so it's clear what has been changed.

[timcharper]

Really perplexed by the failure here. Wondering if there are some maven issues happening upstream.

[chrhicks]

yeah strange... this has happened before. I'll check out the branch locally and if everything passes i'll merge tonight. Unless there is more you want to add (let me know).