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

Accept custom HTTP headers when reading over HTTP(S) #272

Merged
merged 15 commits into from
Jul 18, 2019
Merged

Accept custom HTTP headers when reading over HTTP(S) #272

merged 15 commits into from
Jul 18, 2019

Conversation

ampersand-five
Copy link
Contributor

Http calls woudn't take headers before. I added two changes that allows http calls to now accept user defined headers for http calls.

@ampersand-five
Copy link
Contributor Author

Does this version look good?

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.

Looks good. Left you a minor comment. Please add unit tests.

@ampersand-five
Copy link
Contributor Author

ampersand-five commented Apr 4, 2019

I added a unit test, but I wasn't able to understand why it didn't run (after reading the travis ci output). It seems like I did the exact same thing as the other unit tests. So I don't know why it failed.

@ampersand-five
Copy link
Contributor Author

@mpenkov @piskvorky Any help? The output looks a little different than I remember from a couple months ago, but any points can still help = )

@mpenkov
Copy link
Collaborator

mpenkov commented Jul 17, 2019

@canada11 Have you tried running the test locally? Does it pass?

Copy link
Owner

@piskvorky piskvorky left a comment

Choose a reason for hiding this comment

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

Minor code style comments.

@mpenkov mpenkov changed the title Accept http headers Accept custom HTTP headers when reading over HTTP(S) Jul 18, 2019
@mpenkov
Copy link
Collaborator

mpenkov commented Jul 18, 2019

OK, this looks good to merge.

@canada11 Thank you for your efforts, and congrats on your first contribution to smart_open 🥇 Keep them coming!

@interpolatio Thanks for helping us work out the unit test problems.

@mpenkov mpenkov merged commit 0f17449 into piskvorky:master Jul 18, 2019
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