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

Added support for parsing of doubles and floats. #1065

Merged
merged 2 commits into from
May 13, 2019

Conversation

alexb-uk
Copy link
Contributor

@alexb-uk alexb-uk commented May 11, 2019

Also package-lock updated due to out-of-date version currently checked into master.

Minor tweak to existing tests to better cover the parsing of double and float.

Background:

After reviewing the existing very old PR that fixes doubles and floats thought best to create a new one with a slightly different approach

See: #985

I've gone for Number(text) instead of parseFloat(text) as I think it's safer to throw an error on unexpected characters rather than allowing an assumption.

E.g. "1a" !== 1

There is a good explanation of the pro's con's of Number() vs parseFloat() here: https://stackoverflow.com/questions/12227594/which-is-better-numberx-or-parsefloatx

package-lock updated due to out-of-date version.
@coveralls
Copy link

coveralls commented May 11, 2019

Coverage Status

Coverage increased (+0.004%) to 93.33% when pulling afd8e14 on alexb-uk:master into b089134 on vpulim:master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.004%) to 93.33% when pulling 5aa056e on alexb-uk:master into b089134 on vpulim:master.

@jsdevel
Copy link
Collaborator

jsdevel commented May 11, 2019

please revert package-lock.json. we'll update it.

@alexb-uk
Copy link
Contributor Author

alexb-uk commented May 11, 2019

please revert package-lock.json. we'll update it.

Done, thanks a lot @jsdevel

@alexb-uk
Copy link
Contributor Author

I'm a little confused as to why Travis failed for Node v9 and v7, is that a known issue?

@alexb-uk
Copy link
Contributor Author

If it helps the build passes on my branch so guess it was an intermittent test timeout?

See: https://travis-ci.org/alexb-uk/node-soap/builds/531663881

@jsdevel jsdevel merged commit f7e2efc into vpulim:master May 13, 2019
@jsdevel
Copy link
Collaborator

jsdevel commented May 13, 2019

thanks @alexb-uk !

lfantone pushed a commit to flybondi/node-soap that referenced this pull request May 20, 2019
* Fixed some issues with xsd elements (vpulim#1057)

* Fixed some issues with xsd elements

* Made the fix more specific to the particular usecase

* Added a test for aliased namespaces

* Release v0.27.0

* Move @types/request to dependencies (vpulim#1059)

soap's d.ts files depend on request. Unfortunately, since request
doesn't ship its own types, that means consumers of soap also need
@types/request. Currently @types/request is just a dev dependency, which
doesn't get installed by `npm install`. The fix is to move
`@types/request` to the dependencies list.

The error looks like this:

```
node_modules/soap/lib/client.d.ts(4,26): error TS7016: Could not find a declaration file for module 'request'. '../../../tests/cases/user/soap/node_modules/request/index.js' implicitly has an 'any' type.
```

Note that this only shows up when consumers compile with --strict, which is
fairly common.  Typescript's user-code tests include soap and detected
it after 0.27 shipped.

* Release v0.27.1

* Updated read me to reflect changes in soap.listen (vpulim#1060)

* Updated Read.me to reflect changes in soap.listen

* Updated Readme.md

* Updated Readme.md

* Updated Readme.md

* types: move forceSoap12Headers to IWsdlBaseOptions (vpulim#1063)

"forceSoap12Headers" works on server since bcc41e6. So, this option
should be available to server as well.

* client.addSoapHeader() dynamic SOAP header (vpulim#1062)

* Added support for parsing of doubles and floats. (vpulim#1065)

* Added support for parsing of doubles and floats.
package-lock updated due to out-of-date version.

* Reverting changed package-lock file.
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.

3 participants