Skip to content

Conversation

@bajtos
Copy link
Member

@bajtos bajtos commented Sep 12, 2016

Add tests describing behaviour for numbers with a leading zero, see #143.

  • When the argument's type is any, strong-remoting will treat integer numbers starting with 0 as string, e.g. 0668 will preserve the leading zero. However, floats are still converted to numbers, e.g. both 0.42 and 00.42 are converted to a number.
  • When the argument's type is number or integer, strong-remoting will treat integer numbers starting with 0 as numbers and discarding any extra leading zeroes.

@richardpringle @Amir-61 could one of you please review?

Connect to #312

@Amir-61
Copy link
Member

Amir-61 commented Sep 12, 2016

Nice LGTM :shipit:

@Amir-61
Copy link
Member

Amir-61 commented Sep 12, 2016

Regarding ubunto-0.12 it says:

[sl-ci-run INFO  81.48 ]: **  Install dependencies  : FAIL   [required]
[sl-ci-run INFO  81.48 ]: **  Linter                : FAIL   [optional]
[sl-ci-run INFO  81.48 ]: **  Generate artifacts    : pass   [required]
[sl-ci-run INFO  81.48 ]: **  Run tests             : pass   [required]
[sl-ci-run INFO  81.48 ]: **  Coverage report       : pass   [optional]

I have no strong opinion about that failure. If irrelevant please ignore it. I leave the decision to you.

Also I assume it needs to be back-ported too.

@bajtos
Copy link
Member Author

bajtos commented Sep 13, 2016

Regarding the CI failure:

http://ci.strongloop.com/job/strong-remoting/label=ubuntu-0.12/1423/console

> phantomjs-prebuilt@2.1.12 install /mnt/jenkins/workspace/strong-remoting/cdcdef7b/node_modules/karma-phantomjs-launcher/node_modules/phantomjs-prebuilt
> node install.js

module.js:338
    throw err;
          ^
Error: Cannot find module 'extend'
    at Function.Module._resolveFilename (module.js:336:15)
    at Function.Module._load (module.js:278:25)
    at Module.require (module.js:365:17)
    at require (module.js:384:17)
    at Object.<anonymous> (/mnt/jenkins/workspace/strong-remoting/cdcdef7b/node_modules/request/index.js:17:29)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Module.require (module.js:365:17)

@rmg I thought you had already fixed this issue and your fix was accepted upstream. What am I missing here?

Anyways, the failure is not related to this patch, I'll go ahead and merge.

@bajtos bajtos merged commit 75280a8 into master Sep 13, 2016
@bajtos bajtos deleted the fix/coercion-edge-cases branch September 13, 2016 07:54
@bajtos bajtos removed the #review label Sep 13, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants