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.

Sign up for GitHub

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

Jump to bottom

switched direct requests call to use of session instance #3

Merged
merged 1 commit into from
Dec 23, 2016
Merged

switched direct requests call to use of session instance #3

merged 1 commit into from
Dec 23, 2016
+11 −9

Conversation

guyzmo
Copy link
Contributor

@guyzmo guyzmo commented Dec 23, 2016

No description provided.

@coveralls
Copy link

coveralls commented Dec 23, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 92.434% when pulling 9a8f012 on guyzmo:feature/requests_session into 4bbc50e on unfoldingWord-dev:master.

@guyzmo
Copy link
Contributor Author

guyzmo commented Dec 23, 2016 •
edited
Loading

This patch is necessary for unit tests of guyzmo/git-repo, as the betamax library hijacks a requests session to record and replay HTTP calls.

Basically, what is needed is to be able to use and extract:

gogs = gogs_client.GogsApi(url)
do_stuff_with_session(gogs._requestor.session)

A cleaner approach (to avoid exposing private API), would be to change the entry point object:

session = requests.Session()
gogs = gogs_client(url, session=session)
do_stuff_with_session(session)

but first way (which I currently implemented so it's frictionless) is good enough.

ethantkoenig
ethantkoenig suggested changes Dec 23, 2016
View reviewed changes
gogs_client/_implementation/http_utils.py
@@ -11,8 +11,9 @@ class RelativeHttpRequestor(object):
A thin wrapper around the requests module that allows for endpoint paths
to be given relative to a fixed base URL
"""
def __init__(self, base_url):
def __init__(self, base_url, session=requests.Session()):
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would be safer to have session=None, and then check if session is None inside the constructor. If for some reason I created two GogsApi obects (each using the default session), they would be sharing the same request.Session() object.

In general, having mutable default arguments is risky business in Python.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually that's on purpose, because it's more likely you'd want to keep the same shared session object even between instances. And BTW, it's matching the original behaviour where you were using the global requests context 😉

Copy link
Contributor Author

Choose a reason for hiding this comment

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

anyway, fixed it to your liking 👌

@ethantkoenig
Copy link
Contributor

ethantkoenig commented Dec 23, 2016 •
edited
Loading

@guyzmo I like the suggestion; I'm a big fan of injection!

However, in order to expose the session object in a clean way, doesn't the constructor of the GogsApi object need to take a (perferably optional) session argument to pass along to RelativeHttpRequestor?

@guyzmo guyzmo force-pushed the feature/requests_session branch from 9a8f012 to 7a6378d Compare December 23, 2016 07:20
@coveralls
Copy link

coveralls commented Dec 23, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 92.434% when pulling 7a6378d on guyzmo:feature/requests_session into 4bbc50e on unfoldingWord-dev:master.

@guyzmo
switched direct requests call to use of session instance
c88f317 
Signed-off-by: Guyzmo <guyzmo+github@m0g.net>
@guyzmo guyzmo force-pushed the feature/requests_session branch from 7a6378d to c88f317 Compare December 23, 2016 07:26
@coveralls
Copy link

coveralls commented Dec 23, 2016 •
edited
Loading

Coverage Status

Coverage increased (+0.02%) to 92.434% when pulling c88f317 on guyzmo:feature/requests_session into 4bbc50e on unfoldingWord-dev:master.

@ethantkoenig
Copy link
Contributor

ethantkoenig commented Dec 23, 2016 •
edited
Loading

LGTM. Thanks for the contribution!

@ethantkoenig ethantkoenig merged commit da455d1 into unfoldingWord-dev:master Dec 23, 2016
@guyzmo guyzmo mentioned this pull request Dec 28, 2016
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ethantkoenig ethantkoenig

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@guyzmo @coveralls @ethantkoenig