Added support for numerical ngTrueValue and ngFalseValue attributes

[simonsparks]

feat(ng): support for numerical ngTrueValue and ngFalseValue attributes

Checkbox input fields may now be declared with numerical ng-true-value and ng-false-value settings.
The input's corresponding model will be interpreted as a number rather than a string representation.
Regular string configurations which do not parse as numbers will be treated as before.

BREAKING CHANGE: numeric values for ngTrueValue and ngFalseValue attributes are
now interpreted as numbers rather than their string representation. This may affect
cases where a string representation of a number is expected in a checkbox field's
associated model and the identity operator (===) is used for comparison.

To migrate the code, make comparisons with the numerical typed value or use the
equality operator (==) in place of the identity operator to ensure type invariant value
comparison.

Before:

    if(scope.value === '1') {
      ...
    }

After:

    if(scope.value === 1) {
      …
    }
    // or
    if(scope.value == 1) {
      ...
    }
    // or
    if(scope.value == '1') {
      ...
    }

[petebacondarwin]

PR Checklist (Minor Feature)

• Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name for cross reference)
• Feature improves existing core functionality
• API is compatible with existing Angular apis and relevant standards (if applicable)
• PR doesn't contain a breaking change
• PR contains unit tests
• PR contains e2e tests (if suitable)
• PR contains documentation update
• PR passes all tests on Travis (sanity)
• PR passes all tests on ci.angularjs.org (cross-browser compatibility)
• PR is rebased against recent master
• PR is squashed into one commit per logical change
• PR's commit messages are descriptive and allows us to autogenerate release notes (required commit message format)
• All changes requested in review have been implemented

[petebacondarwin]

@simonsparks - thanks for making this PR. Can you take a look at the task list above.
In particular you need to sign the CLA and fix up the commit message.
Cheers, Pete

[petebacondarwin]

Can this be done more concisely with:

return isString(val) ?
           (isFinite(val) ? parseFloat(val) : val) :
           defaultVal;

If it is a string and finite number (isFinite parses strings and returns false for NaN) then return the parsed number. Also isFinite is more choosy - for instance parseFloat will return 9 for "9/3", whereas isFinite will return false

[simonsparks]

The approach committed I've used in the past based on this SO question: http://stackoverflow.com/questions/18082/validate-numbers-in-javascript-isnumeric

But I don't personally know if ordering isFinite() first will be sufficient to catch all cases where parseFloat might fail.

[petebacondarwin]

Here are most, of the cases I could come up with:

isFinite(100) => true
isFinite(0) => true
isFinite(-100) => true
isFinite(1.3) => true
isFinite(1/0) => false
isFinite(NaN) => false
isFinite("100") => true
isFinite("-34.3") => true
isFinite(null) => true
isFinite(undefined) => false
isFinite("abc") => false
isFinite(1e3) => true
isFinite("1e3") => true

[simonsparks]

Looking further, I noticed a suite of unit tests against various approaches: http://dl.dropboxusercontent.com/u/35146/js/tests/isNumber.html
and saw that isFinite(String(n)) passes empty strings and whitespace as valid numbers.
I'm guessing we could assume a use case where an empty string was desirable as a false value.

[petebacondarwin]

Fair enough. But if you use your original version we should cache the
parsed value rather than running parseFloat twice

On 17 July 2013 14:04, Simon Sparks notifications@github.com wrote:

In src/ng/directive/input.js:

@@ -666,6 +663,13 @@ function checkboxInputType(scope, element, attr, ctrl) {
});
}

+function typedValue(val, defaultVal) {

• return isString(val)
• ? !isNaN(parseFloat(val)) && isFinite(val)

•  ? parseFloat(val)
  

Looking further, I noticed a suite of unit tests against various
approaches:
http://dl.dropboxusercontent.com/u/35146/js/tests/isNumber.html
and saw that isFinite(String(n)) passes empty strings and whitespace as
valid numbers.
I'm guessing we could assume a use case where an empty string was
desirable as a false value.

—
Reply to this email directly or view it on GitHubhttps://github.com//pull/3251/files#r5241172
.

[petebacondarwin]

There is a potential breaking change, where the user may have wished to actually have a string but that string happens to be a number. I don't think this is really an issue because in reality the number will be serialized back to a string as needed.

[petebacondarwin]

We also need a documentation update to explain this new behaviour.

[simonsparks]

@petebacondarwin - no problem, hope it helps.

CLA has been signed (Simon Sparks) and the PR message has been modified, let me know if that still needs improving.

I did wonder about the potential breaking change you mentioned. I thought an extra attribute to express which way to interpret numbers was messy and I think you're probably right about serialization.

Where would we need updated documentation for this change? Just the doc comments in input.js?
Is it acceptable to provide the doc update as a separate commit in this case?

[petebacondarwin]

@simonsparks - thanks for the quick response. Create a new commit with the doc update - yes in the comments input.js. We should probably add a qualified breaking change comment to this new commit explaining the small potential for problems if the developer was relying upon the "type" of the value to be a string, when it contains only numerals. I can squash the two commits together when we merge. In the meantime we ought to get another team member to review this.

[petebacondarwin]

The place to update the docs is probably here: https://github.com/Sparks-Creative-Limited/angular.js/blob/0a2df367a1f1b3abb04d1912f4626e5a7b368c6e/src/ng/directive/input.js#L343

[simonsparks]

I have now refactored the number parsing algorithm to cache the intermediate float value for reuse, I have added some more unit tests to cover different string and number value scenarios, and I have included a breaking change notice in the PR message (not sure about the markup formatting there; I was trying to copy layout in the message conventions guide but it looks like it may need adjusting).
I also noticed that the CI build failed on my commit but it looks like it may be an unrelated issue:

Warning: Error minifying build/angular-resource.js� Use --force to continue.

Aborted due to warnings.

The previous angular.js build in the queue failed for the same reason.

[simonsparks]

The build is now passing.

[simonsparks]

Is this feature good to go now, or is there anything else I need to do?

[btford]

@simonsparks Thanks for the PR!

After some deliberation, we've unfortunately decided not to merge this for a few reasons:

• We think the functionality is good, but we want a more generic solution, not just a one-off for numbers. It would be nice to support other types (or even expressions) as well, but that introduces additional complexities that we have to think about more thoroughly.
• The extra code to coerce to the type you want is pretty minimal. (Fun tip: you can use $scope.value = +$scope.value for floats or $scope.value = ~~$scope.value for ints to coerce the type)
• Therefore it's not worth introducing a breaking change.

Still, we really appreciate the effort you put into the PR, and hope this doesn't discourage you from future contributions.

Thanks!

[simonsparks]

No problem Brian,
My personal use case would benefit more from expressions anyway so I look forward to a more generic solution in the future.

[mattgaffin]

Its not quite so easy if your DB stores integers and you can't (or are not willing) to make your local model pretend to be a string. Any news on the current progress of an official solution? Would be much appreciated! Until then, I guess we'll use a custom directive.