Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add `toNumber` and cleanup `toString`. Plus a wide range of tests. #1577

Closed
wants to merge 41 commits into from

Conversation

2 participants
@Xotic750
Copy link

commented Oct 28, 2015

Xotic750 and others added some commits Oct 28, 2015

Xotic750
Merge pull request #1576 from Xotic750/master
Preserve sign of `0`.
Xotic750
Add `toNumber` and a wide range of tests.
#1574
Cleanup `toString` and added a wide range of tests.
#1576 (comment)
@Xotic750

This comment has been minimized.

Copy link
Author

commented Oct 28, 2015

Ok, something funky with node v0.8, but don't have the time to investigate just now, must sleep and have to take kids to museum tomorrow. Maybe the hex bug that I mentioned in my enhancement post. I'm also unsure if QUnit's deepEqual test for -0

Xotic750 added some commits Oct 28, 2015

Xotic750
Merge remote-tracking branch 'upstream/master'
# Conflicts:
#	lodash.js
#	test/test.js
Xotic750
@Xotic750

This comment has been minimized.

Copy link
Author

commented Oct 28, 2015

That should cover most of the basic use cases, and catch any basic bugs/deficiencies. There are plenty more that could be added to catch other bugs, like I mentioned with Opera where an object has valueOf defined but it's not a function. Other (highly)possible environment deficiencies are regarding whitespace, where the native trim does not trim correctly. Which some could be corrected by if value is string then return +trimmed rather than fall through to return +value. This still leaves return +value coercion open to environment deficiencies.

Xotic750
@Xotic750

This comment has been minimized.

Copy link
Author

commented Oct 28, 2015

To catch even more string issues, the type value === 'string' could be extended to String objects by using isString(value) instead.

@Xotic750

This comment has been minimized.

Copy link
Author

commented Oct 28, 2015

Finally (for now), do we keep this early return for number types?

    function toNumber(value) {
      // Possibly not required, not sure if this improves performance or not?
      if (typeof value === 'number') {
        return value;
      }

Xotic750 added some commits Oct 28, 2015

Xotic750
Be specific about testing `+0`.
Add tests for Boolean objects.
Add test for RegExp object.
Xotic750
@Xotic750

This comment has been minimized.

Copy link
Author

commented Oct 28, 2015

So, node v0.8 has a problem with this case again converting bad hex strings.

Xotic750
Add tests for objects that return binary and octal literal strings, c…
…urrent they fall through to return +value. Should find deficiencies in environments.
@Xotic750

This comment has been minimized.

Copy link
Author

commented Oct 28, 2015

Now need to find a simple fix without implementing a full toPrimitive shim, if possible.

lodash.js Outdated
if (reIsOctal.test(trimmed)) {
return parseInt(trimmed.slice(2), 8);
}
if (reIsBadHex.test(trimmed)) {

This comment has been minimized.

Copy link
@jdalton

jdalton Oct 28, 2015

Member

Shouldn't bad hex fall out naturally as NaN from the +trimmed below?

This comment has been minimized.

Copy link
@Xotic750

Xotic750 Oct 28, 2015

Author

I will look at improving the flow.

lodash.js Outdated
if (reIsBadHex.test(trimmed)) {
return NaN;
}
return +trimmed;

This comment has been minimized.

Copy link
@jdalton

jdalton Oct 28, 2015

Member

Is the return+trimmedneeded at this point could it just rely on+value`?

This comment has been minimized.

Copy link
@Xotic750

Xotic750 Oct 28, 2015

Author

I will look at improving the flow.


QUnit.test('should convert string objects with whitespace accurately', function(assert) {
assert.expect(75);

This comment has been minimized.

Copy link
@jdalton

jdalton Oct 28, 2015

Member

This seems like a large amount of tests for this method. Can you go through this and weed out anything extra.

This comment has been minimized.

Copy link
@Xotic750

Xotic750 Oct 28, 2015

Author

I will do, just making sure that I catch anything in common use.

assert.deepEqual(_.toNumber({
valueOf: '1.1'
}), NaN);
assert.strictEqual(_.toNumber({

This comment has been minimized.

Copy link
@jdalton

jdalton Oct 28, 2015

Member

Same here way too many tests for this, confirm toString behavior and that's enough.

This comment has been minimized.

Copy link
@Xotic750

Xotic750 Oct 28, 2015

Author

I should be able to roll it into something much smaller once I make it robust for the environments that you are testing on Travis. toNumber and numbers to string are something that I use a lot and are important to me. ;)

Xotic750 and others added some commits Oct 30, 2015

Xotic750
Xotic750
Remove optional `length` argument.
Clamp to range 0 to 2^32-1 (MAX_ARRAY_LENGTH), to match other methods.
Xotic750
Merge pull request #1594 from Xotic750/reIsUint
Improve `reIsUint` to filter strings like `0001 and add tests.
Merge pull request #1592 from Xotic750/toLength
Add `toLength` and basic tests.
@jdalton

This comment has been minimized.

Copy link
Member

commented Oct 30, 2015

Can you rebase this PR.

Xotic750
Merge remote-tracking branch 'upstream/master'
# Conflicts:
#	lodash.js
#	test/test.js

@jdalton jdalton force-pushed the lodash:master branch 2 times, most recently from f35a15b to 39a22bf Oct 31, 2015

@Xotic750

This comment has been minimized.

Copy link
Author

commented Oct 31, 2015

I've created a (more) optimised version (I think a little more could be done), instead of calling lodash methods. I don't know if you'd prefer this instead, as it looks like I need to rebase again?

    function toNumber(value) {
      if (value != null) {
        var type = typeof value;
        if (type == 'number') {
          return value;
        }
        if (value) {
          var tagString = objToString.call(value);
          if (tagString != numberTag) {
            if (tagString == stringTag) {
              value += '';
            } else {
              if ((type == 'object' || type == 'function') && isFunction(value.valueOf)) {
                value = value.valueOf.call(value);
                type = typeof value;
                if (type == 'number') {
                  return value;
                }
              }
              if (value && (type == 'object' || type == 'function')) {
                value = toString(value);
              }
            }
            if (value && typeof value == 'string') {
              value = trim(value);
              if (value) {
                var isBinary = reIsBinary.test(value);
                if (isBinary || reIsOctal.test(value)) {
                  return nativeParseInt(value.slice(2), isBinary ? 2 : 8);
                }
                if (reIsBadHex.test(value)) {
                  return NaN;
                }
              }
            }
          }
        }
      }
      return +value;
    }
@jdalton

This comment has been minimized.

Copy link
Member

commented Oct 31, 2015

Not a fan of that. Less is more.

}
return +value;
}

This comment has been minimized.

Copy link
@jdalton

jdalton Oct 31, 2015

Member

This can be simplified:

function toNumber(value) {
  var result = +value;
  if (typeof value == 'number' || result === result) {
    return result;
  }
  value = trim(value);
  var isBinary = reIsBinary.test(value);
  return (isBinary || reIsOctal.test(value))
    ? nativeParseInt(value.slice(2), isBinary ? 2 : 8)
    : result;
}

This comment has been minimized.

Copy link
@Xotic750

Xotic750 Oct 31, 2015

Author

That is going to fail on at least node v0.8 I believe. But that should just be a case of adding the bad hex test back in. It's nice to see where your experience pays off wrt simplification. Let me change it and we can see.

This comment has been minimized.

Copy link
@jdalton

jdalton Oct 31, 2015

Member

Ah cool, verified 0.8 does not return NaN :P

@Xotic750

This comment has been minimized.

Copy link
Author

commented Oct 31, 2015

Ok, I'll just rebase it.

@Xotic750

This comment has been minimized.

Copy link
Author

commented Oct 31, 2015

I don't know if it is just me, but git doesn't seem very good at auto merging simple upstream changes? I seem to need to do a fair amount of manual merging each time. And it always worries me that I may get it wrong. :)

@jdalton

This comment has been minimized.

Copy link
Member

commented Oct 31, 2015

It'll be easier in your branch to copy the lodash.js and test.js to another location then.

git reset --hard c142cfc81dce4c3cbd3ff4ceb33e13df13062be9
# add your lodash.js and test.js back
git add -u
git commit -m "Add _.isNumber."
git push origin +master
Xotic750
Merge remote-tracking branch 'upstream/master'
# Conflicts:
#	lodash.js
#	test/test.js
@Xotic750

This comment has been minimized.

Copy link
Author

commented Oct 31, 2015

I guess I'll get better at it, at the end of the day I'm just a self taught enthusiast. ;) Node v0.8 and v0.10 don't seem happy with that new code.

@jdalton

This comment has been minimized.

Copy link
Member

commented Oct 31, 2015

Closed via 3795534.

@lock

This comment has been minimized.

Copy link

commented Dec 27, 2018

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@lock lock bot locked as resolved and limited conversation to collaborators Dec 27, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.