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

Interface for savon-multipart for Savon2 #434

Merged
merged 4 commits into from
May 5, 2013

Conversation

tjarratt
Copy link
Contributor

After much deliberation in the comments for issue 424 I am proud to present some support in Savon 2 for the multipart-savon gem.

The motivation behind this change is to provide the savon-multipart gem a hook
so that one can use it with savon. There is little desire (or much reason) to
fix this directly in Savon since very few people will ever use it. So long as
there is a mechanism for end users of savon-multipart to specify this, it should
be easy for Savon to refactor without needing to make changes for multipart
support.

The indended use case for this would be to either configure a global or local
multipart key to :true like so:

client = Savon::Client('http://service.example.com', :multipart => true)

# or for only a single call on a client
client.call(:some_method_returns_multipart, :multipart => true)

The tests here only test that we return an object that is an instance of a Savon::Multipart::Response, but does not assert on the functionality of that class. I currently have a fork of savon-multipart with those changes that might be interesting to view alongside these.

Any feedback is greatly appreciated! Many thanks for @rubiii for taking the time to read my incoherent ramblings and help design a good interface. My own regret is that it took me so long to formalize this!

The motivation behind this change is to provide the savon-multipart gem a hook
so that one can use it with savon. There is little desire (or much reason) to
fix this directly in Savon since very few people will ever use it. So long as
there is a mechanism for end users of savon-multipart to specify this, it should
be easy for Savon to refactor without needing to make changes for multipart
support.

The indended use case for this would be to either configure a global or local
multipart key to :true like so:

client = Savon::Client('http://service.example.com', :multipart => true)

# or for only a single call on a client
client.call(:some_method_returns_multipart, :multipart => true)
I had forgot that class_variable_set was a private method before ruby 1.9
@rubiii
Copy link
Contributor

rubiii commented Apr 18, 2013

hey @tjarratt, thanks for taking the time to work on this! your change looks good. i would probably change
a few details, but i like it. i can dedicate some time to open issues starting tomorrow and will make sure to
follow up on this.

ps. did you remove your fork of savon-multipart from github?

@tjarratt
Copy link
Contributor Author

Thanks for taking a look @rubiii ; I transferred ownership of my fork to my company's repository. You can find it here.

If I understand pull requests on github correctly, I think existing pull requests will work even after you transfer a repository. Sorry for the confusion!

@rubiii
Copy link
Contributor

rubiii commented Apr 19, 2013

thanks. i was looking for the changes to savon-multipart (which moved here).
i left some comments on your commits.

@rubiii
Copy link
Contributor

rubiii commented Apr 19, 2013

regarding the changes to savon, i think i would rather raise an error in case multipart was enabled and savon-multipart could not be loaded instead of storing the result in the @@supports_multipart class variable which is shared between all clients. could you change that?

@tjarratt
Copy link
Contributor Author

Yeah, that sounds eminently reasonable.

Don't try to load it, assume that the end user will do this - this way
we don't need to potentially call require multiple times during runtime.

Also fixed the multipart specs so they'd be less likely to interfere with
other ones and make those fail. Instead of mucking with the spec-level
globals object, create one for multipart when we need it.
@rubiii
Copy link
Contributor

rubiii commented Apr 20, 2013

thanks! let me know when this works with the new savon-multipart.

@tjarratt
Copy link
Contributor Author

This all works with the new savon-multipart. I've been waiting to issue a pull request for savon-multiplart until this makes it in.

@rubiii
Copy link
Contributor

rubiii commented Apr 23, 2013

@tjarratt could you open the pull request and link it to this issue so i can take another look at the changes and easily test both in integration?

rubiii added a commit that referenced this pull request May 5, 2013
Interface for savon-multipart for Savon2
@rubiii rubiii merged commit 8009a22 into savonrb:master May 5, 2013
rubiii added a commit that referenced this pull request May 5, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants