Update behaviour of {{ifNth}} helper to directly use value given, resolves #228 #229

Merged
merged 2 commits into from Jan 8, 2017

Projects

None yet

4 participants

@huntie
Contributor
huntie commented Sep 17, 2016 edited

The ifNth helper will now render a block if the remainder is zero when operand a is divided by b. Previously, this parameter was incremented before comparison, which led to incorrectly described and confusing behaviour. #228

Whilst considering, please note that this is a breaking change, although the implementation now matches the published documentation.

@huntie huntie Update behaviour of {{ifNth}} helper to directly use value given 6f081c8
lib/comparison.js
@@ -277,7 +277,7 @@ helpers.ifEven = function(num, options) {
*/
helpers.ifNth = function(a, b, options) {
- if (++b % a === 0) {
+ if (b % a === 0) {
@jonschlinkert
jonschlinkert Sep 21, 2016 Member

perhaps we should either cast a and b to numbers and/or do a check to ensure a and b are numbers before line 280?

@huntie
huntie Sep 22, 2016 edited Contributor

None of the adjacent helpers in the file are doing any casting/checks at present, however if you have an example of a helper that implements type checking and appropriate error messages I'll be happy to add it in.

@doowb
doowb Sep 22, 2016 Member

This validates that an array is passed in: https://github.com/assemble/handlebars-helpers/blob/master/lib/comparison.js#L206

These validate the values are numbers in the utils:
https://github.com/assemble/handlebars-helpers/blob/master/lib/comparison.js#L261
https://github.com/assemble/handlebars-helpers/blob/master/lib/comparison.js#L305

I think that checking that both are utils.isNumber should be sufficient.

@huntie
huntie Sep 22, 2016 Contributor

Awesome, will update shortly.

@huntie
huntie Oct 5, 2016 Contributor

This was updated on 22 September :)

@huntie huntie Add type checking for {{isNth}} integer parameters 9c8ca9b
@AurelioDeRosa

Is this PR good to merge?

@jonschlinkert jonschlinkert merged commit c8980ae into helpers:master Jan 8, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment