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

Follow 302 redirects, don't mix quotes #577

Merged
merged 1 commit into from
Mar 1, 2015
Merged

Follow 302 redirects, don't mix quotes #577

merged 1 commit into from
Mar 1, 2015

Conversation

DeviaVir
Copy link
Contributor

This change allows a WSDL and SOAP request to follow redirects returned from the servers (yes, I have to work with some freaky API's).

  • Runs itself again to follow the redirect (replacing ':80' and ':443' to make sure the request will actually work)
  • Replaces all double quotes by single quotes, since there was a mixed use

@herom
Copy link
Contributor

herom commented Feb 27, 2015

Hello @DeviaVir and sorry for the long delay - I'm really short on schedule in 2015 😞

Thanks a lot for your contribution, this seems like a viable addition to our library and we appreciate it a lot ! 👍

In order to get your PR merged, I'll have to ask you to add tests for your code change, as otherwise we can't easily identify and prevent regressions, etc. Please have a look at our Guidelines on Submitting a Pull Request, in advance 😃

@DeviaVir
Copy link
Contributor Author

Hi @herom, understandable! I'll take a look at your tests and will see if I can add one or two for this use case. Thanks for your feedback!

@DeviaVir
Copy link
Contributor Author

DeviaVir commented Mar 1, 2015

@herom I found an even simpler way of accomplishing this:
https://github.com/vpulim/node-soap/pull/577/files#diff-1c0f1c434b17b7f8795d44a51a14320aR42

I ran your test suite and had 25 errors to start with, and didn't notice any tests that already do something with a stubbed server. Now that this change is down to a oneliner (if you ignore the replaced quotes and the added var), do you still require tests?

@DeviaVir
Copy link
Contributor Author

DeviaVir commented Mar 1, 2015

@herom
Copy link
Contributor

herom commented Mar 1, 2015

@DeviaVir thanks a lot for keeping track and don't loose interest 👍

herom added a commit that referenced this pull request Mar 1, 2015
Follow 302 redirects, don't mix quotes
@herom herom merged commit 965a897 into vpulim:master Mar 1, 2015
@DeviaVir DeviaVir deleted the feature/302 branch March 1, 2015 18:17
diarmaidm pushed a commit to diarmaidm/node-soap that referenced this pull request Feb 3, 2016
Follow 302 redirects, don't mix quotes
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 5287a5a on DeviaVir:feature/302 into * on vpulim:master*.

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