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

Issue 537: Take namespaces configured to be ignored into account #662

Merged
merged 1 commit into from
May 26, 2015
Merged

Issue 537: Take namespaces configured to be ignored into account #662

merged 1 commit into from
May 26, 2015

Conversation

tobias-neubert
Copy link
Contributor

Please take a thorough look into the code because I had to fix 3 tests after I changed the wsdl.js. Those three tests are at the same time the tests to proof my commit works as expected. I do not understand the purpose of the shouldIgnoreNamespace method that I deleted in this commit. The reason I deleted it is described below.

Although one can provide an array of namespaces that shall be ignored to createClient(), they are not used in objectToXml(). This commit fixes that.

Moreover it removes the shouldIgnoreNamespace() method because it makes no sense to explicitly configure namespaces to be ignored and than let some weird logic calculate if a given namespace is going to be ignored.

@herom
Copy link
Contributor

herom commented May 26, 2015

@tobias-neubert thanks a lot for your effort 👍

The shouldIgnoreNamespace() method was introduced in #506 as a Bugfix for the issue described in #470.

As this introduced another array of ignored namespaces (which is only taken into consideration on the schema) - WSDL.prototype._ignoredSchemaNamespaces I think this property should also be removed if there is no need for the method itself anymore. As the tests are not failing when the method is removed (as in your PR) the method is either not necessary anymore, or the tests were wrong... hmm...

…ount

Although one can provide an array of namespaces that shall be ignored to createClient(), they are not used in objectToXml(). This commit fixes that.

Moreover it removes the shouldIgnoreNamespace() method because it makes no sense to explicitly configure namespaces to be ignored and than let some weird logic calculate if a given namespace is going to be ignored.
@tobias-neubert
Copy link
Contributor Author

Well, the tns namespace was exactly the one I had problems with too. As I said already, I do not really understand what the shouldIgnoreNamespace method tries to find out but, again, I believe that it makes no sense to have any kind of automation that overrides an explicit configuration. And as you stated: The tests are green.

Hence, I deleted the _ignoredNamespaces property.

@herom
Copy link
Contributor

herom commented May 26, 2015

Thanks a bunch 👍

I guess, due to the reason of "green tests", we're save to delete/revert this previous PR and move firward savely. If problems because of this change come up, we should address them first with failing tests imho.

herom added a commit that referenced this pull request May 26, 2015
Issue 537: Take namespaces configured to be ignored into account
@herom herom merged commit 18cffb1 into vpulim:master May 26, 2015
diarmaidm pushed a commit to diarmaidm/node-soap that referenced this pull request Feb 3, 2016
Issue 537: Take namespaces configured to be ignored into account
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.

2 participants