Skip to content
This repository has been archived by the owner on Nov 6, 2023. It is now read-only.

Add support for StringIO inside body headers #39

Merged
merged 2 commits into from
Aug 1, 2017

Conversation

dixpac
Copy link
Contributor

@dixpac dixpac commented Jul 21, 2017

There is one IO which doesn't inherit from IO, StringIO(inherits from Data).
StringIO is especially useful in testing scenarios

There is one IO which doesnt inherit from IO, StringIO.
StringIO is especially usefull in testing scenarios
@msftclas
Copy link

This seems like a small (but important) contribution, so no Contribution License Agreement is required at this point. We will now review your pull request.
Thanks,
Microsoft Pull Request Bot

Copy link
Member

@yaxia yaxia left a comment

Choose a reason for hiding this comment

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

Thanks. This partially addressed:
#29

@dixpac
Copy link
Contributor Author

dixpac commented Jul 25, 2017

@yaxia 👍, any plans when you are going to merge this. Waiting on this ship 😃

@yaxia
Copy link
Member

yaxia commented Jul 26, 2017

@sarangan12 could you please comment on the release plan? Thanks.

@sarangan12
Copy link
Member

@vishrutshah Please take a look at this PR. In the original issue, you have mentioned to handle IO, File and StringIO. But file is handled only by the else condition. Since you have more context on this issue, I would like you to review

@vishrutshah
Copy link
Contributor

@dixpac We appreciate your effort in improving the SDK. Thank you so much for the contribution. Changes looks good. Would you mind adding a sample unit test case into http_request_test.rb to make sure going forward no one breaks the expectations, please?

Thanks again for taking time to contribute.

@dixpac
Copy link
Contributor Author

dixpac commented Jul 28, 2017

@vishrutshah no problem 👍

@dixpac
Copy link
Contributor Author

dixpac commented Jul 29, 2017

@vishrutshah there you go I've cover all 3 cases in testes 😃

Copy link
Contributor

@vishrutshah vishrutshah left a comment

Choose a reason for hiding this comment

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

LGTM!

@dixpac
Copy link
Contributor Author

dixpac commented Aug 1, 2017

Any plans on release(I need this desperately) 😃 . I just added azure support out of the box inside new rails(activestorage). Currently we are using source from my fork which contains this fix. Would love to depend on official gem directly 🙏

@sarangan12 sarangan12 merged commit 1e7c314 into Azure:master Aug 1, 2017
@sarangan12
Copy link
Member

@dixpac Am getting the release today

@sarangan12
Copy link
Member

@dixpac The release is completed.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants