Ensure a session is passed through to GitHubCore #774
Conversation
Unless a session (or a self) is passed through a new session might be created, which can lead to some interesting things. See sigmavirus24#770 This also adds a session class argument for PullDestination and updates the calls to it. A future commit will make the session argument of GitHubCore nonoptional. Related-to: sigmavirus24#771 Signed-off-by: Jesse Keating <omgjlk@github.com>
59d3762 to
52fe8e0
Compare
github3/github.py
Outdated
| @@ -57,7 +58,8 @@ class GitHub(GitHubCore): | |||
| """ | |||
|
|
|||
| def __init__(self, username='', password='', token=''): | |||
| super(GitHub, self).__init__({}) | |||
| session = GitHubSession() | |||
There was a problem hiding this comment.
One thing that was convenient in the past was not having calls to GitHubSession sprinkled everywhere. What do you think of having a GitHubCore method new_session that returns a new GitHubSession so we can do
super(GitHub, self).__init__({}, self.new_session())
And likewise with other things like GitHubStatus.
There was a problem hiding this comment.
Ah, that could work pretty well. Mostly I was trying to avoid the end user from having to instantiate their own session first, so that they could continue using foo = GitHub() as they do today. Your way should still support that, I'll play around with it.
There was a problem hiding this comment.
Yeah, I agree that the user shouldn't have to instantiate it themselves.
tests/unit/test_issues_milestone.py
Outdated
| @@ -50,14 +52,14 @@ def test_none_creator(self): | |||
| """Show that creator is None when json attribute is set to None.""" | |||
| json = self.instance.as_dict().copy() | |||
| json['creator'] = None | |||
| milestone = github3.issues.milestone.Milestone(json) | |||
| milestone = github3.issues.milestone.Milestone(json, session) | |||
There was a problem hiding this comment.
There's a create_session method on our UnitHelper classes. Please use that. It ensures we never accidentally talk to the internet in our unit tests.
There was a problem hiding this comment.
Cool, I was very unsure about these bits in tests. Glad to have your input 👍
tests/unit/test_models.py
Outdated
| @@ -105,7 +107,7 @@ def test_exposes_attributes(self): | |||
|
|
|||
| def test_from_json(self): | |||
| """Verify that method returns GitHubObject from json.""" | |||
| github_core = GitHubCore.from_json('{}') | |||
| github_core = GitHubCore.from_json('{}', session) | |||
We want the session to always be there. This change makes it required and updates the various places that broke. Fixes: 771 Signed-off-by: Jesse Keating <omgjlk@github.com>
52fe8e0 to
c8e256c
Compare
|
I think I addressed the concerns! Please review again @sigmavirus24 |
Unless a session (or a self) is passed through a new session might be
created, which can lead to some interesting things. See #770
This also adds a session class argument for PullDestination and updates
the calls to it.
A future commit will make the session argument of GitHubCore
nonoptional.
Related-to: #771
Signed-off-by: Jesse Keating omgjlk@github.com