Supporting Handlebars subexpressions

[tndev]

I saw this question Handlebars subexpression throws “options.fn is not a function” error on stackoverflow:

The OP was asking why {{#and (gt 4 3) (gt 5 4)}}OK{{/and}} will throw a:

TypeError: options.fn is not a function

In the code it is obvious the helper requires fn and inverse (so an if/else) block:

helpers.gt = function(a, b, options) {
  if (arguments.length === 2) {
    options = b;
    b = options.hash.compare;
  }
  if (a > b) {
    return options.fn(this);
  }
  return options.inverse(this);
};

To support {{#and (gt 4 3) (gt 5 4)}}OK{{/and}} the helper should return true or false instead of options.fn(this) or options.inverse(this) if those blocks don't exists.

This might probably be a interesting feature. Or am I missing something here?

To support subexpressions the code could be changed to this:

helpers.gt = function(a, b, options) {
  if (arguments.length === 2) {
    options = b;
    b = options.hash.compare;
  }

  //fn block exists: it is not a subexpression
  if( options.fn ) {
     if (a > b) {
       return options.fn(this);
     }
     return options.inverse(this);
  } else { // otherwise return the result of the comparison
     return a > b;
  }
};

[assemblebot]

@tndev Thanks for the issue! If you're reporting a bug, please be sure to include:

• The version of assemble you are using.
• Your assemblefile.js (This can be in a gist)
• The commandline output. (Screenshot or gist is fine)
• What you expected to happen instead.

[jonschlinkert]

it's not related to sub-expressions, the helper is a block helper, and it's being used as a non-block helper. I'm not opposed to adding logic to support both if you want to do a pr with unit tests.

[tndev]

I would write the tests according to your isArray tests and place it in the describe('gt', function() { -> describe('non-block helper', function() {?

it('should return true if a > b as a non-block helper.', function() {
    hbs.compile('{{gt 20 15}}')().should.eql('true');
});
it('should return false if a < b as a non-block helper.', function() {
    hbs.compile('{{gt 14 15}}')().should.eql('false');
});
it('should return false if a == b as a non-block helper.', function() {
    hbs.compile('{{gt 14 14}}')().should.eql('false');
});

(the same for all the other comparison helpers)

[jonschlinkert]

I think so, yeah. Feel free to use assert. You can use should if you want, but I've been converting the tests over to use assert, so that's fine too. thanks

[tndev]

If you convert it to assert anyway, then I'll use assert. I just looked around in your tests where you compare to 'true' in a non block helper and adapted that style.

[tndev]

Not sure if I should open an new Issue about that in general, discuss it here or if I should add that as discussion about it with the pull request as soon as I have finished the rewriting.

While doing the rewriting it seem the code might be clearer and easier to maintain, if the comparison functions would always return true or false and then write a utility function like:

module.exports = comparisonHelpers.map(function(helper) {
  return function() {
    var options = arguments[arguments.length - 1];
    var result = helper.apply(this, arguments);
    // used as block helper
    if (options.fn) {
      if (result) {
        return options.fn(this);
      } else {
        return options.inverse(this);
      }
    } else { // used as non-block helper
      return result;
    }
  };
});

[tndev]

I finished the code update for the whole comparison.js and added all tests. Except the ifNth test. Before I'll do the pull request I'll wait for your feedback about the question above.

[jonschlinkert]

@tndev great, I was thinking about this, I like the concept but it would help to see the code so we can discuss. Feel free to do the pr, thx