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 preserveWhitespace option to prevent trimming leading / trailin… #972

Merged
merged 8 commits into from
Oct 2, 2017

Conversation

samuelms1
Copy link
Contributor

…g whitespace from text and cdata values

@samuelms1
Copy link
Contributor Author

My team uses this soap library and we ran into an issue where a soap service we consume has values that begin with a space and the space is intended to be preserved (part of an identifier).

This change shouldn't break any existing behavior, rather it gives the ability to disable trimming whitespace for those who wish to turn that off.

@coveralls
Copy link

coveralls commented Sep 27, 2017

Coverage Status

Coverage increased (+0.01%) to 93.138% when pulling 0c495c4 on samuelms1:master into 01d8585 on vpulim:master.

Copy link
Contributor

@herom herom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add at least 1 test (see our section on How to submit a Pull Request in our CONTRIBUTING.md doc

lib/client.js Outdated
@@ -126,6 +126,7 @@ Client.prototype._initializeOptions = function(options) {
this.streamAllowed = options.stream;
this.wsdl.options.attributesKey = options.attributesKey || 'attributes';
this.wsdl.options.envelopeKey = options.envelopeKey || 'soap';
this.wsdl.options.preserveWhitespace = options.preserveWhitespace || false;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you please use !! to convert into boolean (this.wsdl.options.preserverWhitespace = !!options.preserveWhitespace)?

package.json Outdated
@@ -1,6 +1,6 @@
{
"name": "soap",
"version": "0.21.0",
"version": "0.22.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert the version change - we'd like to change it ourselves when we create the next release (which will be really soon, promised 👍 )

lib/wsdl.js Outdated
@@ -1403,7 +1403,9 @@ WSDL.prototype.xmlToObject = function(xml, callback) {
};

p.oncdata = function (text) {
text = trim(text);
if (!self.options || !self.options.preserveWhitespace) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it's save to check for self.options.preserveWhitespace only as the options should be set up anyways(?)

@herom
Copy link
Contributor

herom commented Sep 29, 2017

please rebase your PR with the latest master - I updated our travis.yml in order to fix the failing node 4.x tests 👍

@coveralls
Copy link

coveralls commented Oct 2, 2017

Coverage Status

Coverage increased (+0.02%) to 93.529% when pulling e1efe4e on samuelms1:master into 6a7a49a on vpulim:master.

@coveralls
Copy link

coveralls commented Oct 2, 2017

Coverage Status

Coverage increased (+0.02%) to 93.529% when pulling a69b923 on samuelms1:master into 6a7a49a on vpulim:master.

@coveralls
Copy link

coveralls commented Oct 2, 2017

Coverage Status

Coverage increased (+0.02%) to 93.529% when pulling b617c2a on samuelms1:master into 6a7a49a on vpulim:master.

@samuelms1
Copy link
Contributor Author

@herom Thanks for the feedback. I think I've addressed it all, but let me know.

@herom
Copy link
Contributor

herom commented Oct 2, 2017

@samuelms1 awesome! thanks a lot for you contribution 💯

Could you please do me a last favour and add a test which guarantees that the whitespaces get cut if the preserverWhitespace option is not set? Thus we have proof that both cases do still work and can't change secretly with further PRs being accepted 👍

@samuelms1
Copy link
Contributor Author

@herom Good idea. I'll add that in now.

@coveralls
Copy link

coveralls commented Oct 2, 2017

Coverage Status

Coverage increased (+0.02%) to 93.529% when pulling a147c6f on samuelms1:master into 6a7a49a on vpulim:master.

@herom
Copy link
Contributor

herom commented Oct 2, 2017

Awesome! 💯 Thanks a ton @samuelms1 👍 😺

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