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

Adding streaming http support. #236

Merged
merged 7 commits into from
Feb 26, 2019
Merged

Conversation

hzeng-otterai
Copy link
Contributor

This pack adds the streaming support for http.
The responses lib doesn't support mock of streaming http so I didn't have a good way to unit test it. Any suggestions?

@menshikh-iv
Copy link
Contributor

menshikh-iv commented Sep 28, 2018

Hi @handsomezebra, I think you can mock it manually with standart python mock.

Also, it's a good idea to add some integration test too (just up simple HTTP server and read file from it)

CC: @mpenkov

@menshikh-iv menshikh-iv requested a review from mpenkov September 28, 2018 07:35
Copy link
Contributor

@menshikh-iv menshikh-iv left a comment

Choose a reason for hiding this comment

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

Looks good, thanks @handsomezebra, please work on testing now.

smart_open/http.py Outdated Show resolved Hide resolved
smart_open/http.py Show resolved Hide resolved
Copy link
Collaborator

@mpenkov mpenkov left a comment

Choose a reason for hiding this comment

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

In general, the code looks very good.

I think we need more tests, especially unit tests for the new SeekableBufferedInputBase class. Seek logic can be tricky, and having tests will insure us against logic bugs in that functionality.

smart_open/http.py Show resolved Hide resolved
smart_open/http.py Outdated Show resolved Hide resolved
@menshikh-iv
Copy link
Contributor

ping @handsomezebra, are you planning to finish PR?
CC @mpenkov

@mpenkov
Copy link
Collaborator

mpenkov commented Feb 26, 2019

Still need to add tests.

@mpenkov mpenkov merged commit 2af2eae into piskvorky:master Feb 26, 2019
@mpenkov
Copy link
Collaborator

mpenkov commented Feb 26, 2019

@handsomezebra I cleaned up your commit for you, added some tests, and merged this in. Congrats on your first PR to smart_open 🚀

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