-
Notifications
You must be signed in to change notification settings - Fork 4
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
feat: 4305 added gspread analytics package #4314
Conversation
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 haven't looked at this fully yet, but one thing that comes to mind is that the version in setup.py
should be updated -- specifically I intended it to be semantic versioning, so I would increase the version to 3.1.0
, since I think this a non-breaking feature change
f7a8e93
to
04d03af
Compare
@hunterckx whoops, thanks for letting me know, I wasn't familiar with setupTools. I bumped the version and added the new dependencies in |
# Build Drive API | ||
drive_credentials = extract_credentials(authentication_response) | ||
gc = gspread.authorize(drive_credentials) | ||
drive_api = build('drive', 'v3', credentials=drive_credentials) |
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 imagine that, since authenticate_gspread
and authenticate_drive_api
exist, they should be used here instead of initializing the API clients directly
|
||
def authenticate_drive_api(authentication_response): | ||
"""Authenticates the Drive API using the credentials in the tuple from api.authenticate""" | ||
return build('drive', 'v3', credentials=extract_credentials(authentication_response)) |
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.
The tuple returned from calling api.authenticate
with drive_service_params
should have the Drive API as its first item, so I don't think calling build
here is necessary
Thanks for those reviews @hunterckx! Let me know if it looks good 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.
Looks good!
Ticket
Closes #4305.
Reviewers
@hunterckx .
@NoopDog
Changes
Known Issues