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

Add 'SOAPAction' header to requests #2

Merged
merged 3 commits into from
Aug 31, 2015
Merged

Add 'SOAPAction' header to requests #2

merged 3 commits into from
Aug 31, 2015

Conversation

SpotlightKid
Copy link
Contributor

Instead of having to add the SOAPAction header (required by spec!) to HTTP requests by setting it on the soap message instance, this change allows to specify the value for this header to be passed when calling a rinse.client.SoapClient instance as an optional second param (defaults to empty string), which is then passed to rinse.message.SoapMessage.request, where it is passed, together with the headers set on the SoapMessage instance, to requests.Request.

So instead of:

msg = SoapMessage(etree.Element('test'))
msg['SOAPAction'] = 'test'
client = SoapClient('<url>')
response = client(msg)

you can now write:

msg = SoapMessage(etree.Element('test'))
client = SoapClient('<url>')
response = client(msg, 'test')

I also fixed a superfluous comma in the request.Request instantiation call.

IMHO, you should also be able to specify the URL on a per-request basis, but that would be another PR.

@tysonclugg tysonclugg self-assigned this Feb 23, 2015
@tysonclugg
Copy link
Owner

Thanks for the PR!

I'd prefer that backwards compatibility is maintained, in which case the default value for action in rinse/client.py line 30 should be None instead of an empty string. As for superfluous commas, I prefer it that way so that adding more parameters to method/function calls doesn't pollute diffs with noise (ie: adding the comma back), and also to keep in the habbit of using commas in tuples so they aren't an exception to the norm. Otherwise, looks OK to me - update your PR and you'll get to keep the credit for the changes. I'd be happy for you to submit another PR for the per-request URL changes.

Cheers,
Tyson.

@SpotlightKid
Copy link
Contributor Author

Thanks for considering the patch!

With regards to the default value of action, I respectfully disagree. The SOAPAction is required by the SOAP 1.1 spec, even if it's empty, so I think compliance beats backwards-compatibility here. I even went so far as adding explicit tests for this now. If you prefer backwards-compatibility, I suggest you accept the PR and then change the line back. I wouldn't want to be recorded as the author of that change.

BTW: in the tests I used the mock library, which I had to add to setup.py in tests_require. If you're not ok with that, I can remove it, but it would make it very hard to test SoapClient.__call__() properly.

About the trailing comma, I think it's bad style, even in dicts, lists and tuples I tend to avoid it, but I don't have strong feelings about this, so I put it back in.

@tysonclugg tysonclugg merged commit 2a3bd14 into tysonclugg:master Aug 31, 2015
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