Skip to content
This repository was archived by the owner on Aug 7, 2024. It is now read-only.

Proxy settings feature added#449

Merged
bear merged 5 commits intobear:masterfrom
mkdadi:master
Mar 13, 2017
Merged

Proxy settings feature added#449
bear merged 5 commits intobear:masterfrom
mkdadi:master

Conversation

@mkdadi
Copy link
Copy Markdown
Contributor

@mkdadi mkdadi commented Mar 8, 2017

This change is Reviewable

@jeremylow
Copy link
Copy Markdown
Collaborator

I have some concerns with this approach.

  • requests can read proxy information from environment variables as explained here: http://docs.python-requests.org/en/master/user/advanced/#proxies , which seems like the simplest approach to adding proxy support since that's just a documentation change pointing out that that's available.

  • rather than adding a new keyword argument to each method (and then remembering to do that on all future methods), we could do something where we add a proxies kwarg to the Api instantiation and then have that be available to Api._RequestUrl() etc. as self.proxies.

@mkdadi
Copy link
Copy Markdown
Contributor Author

mkdadi commented Mar 8, 2017

I agree that requests can read the proxy information from environment variables but if I want only this api to run through proxy I thought this would help. And yes you are correct rather that adding it to every method, it would have been better if an attribute is added to the class, Thanks.

Copy link
Copy Markdown
Collaborator

@jeremylow jeremylow left a comment

Choose a reason for hiding this comment

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

Can we add inline documentation of this parameter to the docstring for the Api.__init__()method?

@mkdadi
Copy link
Copy Markdown
Contributor Author

mkdadi commented Mar 12, 2017

Inline documentation for the parameter has been added. Thanks.

Merging latest commits
Copy link
Copy Markdown
Collaborator

@jeremylow jeremylow left a comment

Choose a reason for hiding this comment

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

Sorry - we do two spaces for docstrings. Other than that I'm good on this and we can get it merged.

@mkdadi
Copy link
Copy Markdown
Contributor Author

mkdadi commented Mar 13, 2017

Done 👍

@bear bear merged commit e02e9b8 into bear:master Mar 13, 2017
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.

3 participants