-
Notifications
You must be signed in to change notification settings - Fork 208
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
gmail: replace deprecated oauth2client package with google-auth-oauthlib #743
gmail: replace deprecated oauth2client package with google-auth-oauthlib #743
Conversation
I'd be especially supportive of adding these files if they were accompanied by an additional section in the contributing docs describing how to use them as an alternative method of setting up a development environment. |
tests/test_gmail.py
Outdated
|
||
self.assertEqual(service.get_credentials().to_json(), expected.to_json()) | ||
|
||
def test_get_credentials_expired(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you add some comment(s) explaining how this test works? It isn't obvious why mocking Credentials.refresh
to return None
would yield the same results as expected.__dict__["expiry"] = datetime.now()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm new to mocking and agree my initial test wasn't very clear, but after sleeping on it I think I have a cleaner/more self explanatory solution for this via 7e4c263. Let me know what you think!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks!
@ryneeverett can this be merged? I was planning on opening a separate PR for the |
I do plan to merge this but I usually wait a few weeks to give others the opportunity to review. Reviews from others are rare these days but I like to leave a window in hopes that this will change. There isn't much risk of merge conflicts at the current rate of development. Please do submit the Pipfile additions in a separate PR. |
Sounds good, thanks! |
Summary
When following the Gmail setup instructions, I discovered that Google's quick start page relies on
google-auth-oauthlib
instead of the deprecatedoauth2client
package, whichbugwarrior
currently uses. Since my credentials had expired, I also discovered that the newcredentials.json
created by following the quick start guide is also incompatible with the old credential file I was using. While I'm no OAuth expert, I used the Python code on the quick start page to make updates tobugwarrior
's Gmail service so we can use the newergoogle-auth-oauthlib
package. Hopefully this saves someone else some trouble! See oauth2client deprecation for more info.Some other clean up items:
gmail
as supported service inREADME
and sort list alphabetically.install_requires
andextras_require
alphabetically insetup.py
.extras_require
to use{}
instead ofdict()
, since the former plays nicer with pipenv, which I use forbugwarrior
development. Seeextras_require
in setup.py is not correctly evaluated pypa/pipenv#4309 for the relevantpipenv
issue. If there's interest, I can open a separate PR with thePipfile
andPipfile.lock
I use on my fork.