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.

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

Feature/google api #10

Merged
merged 11 commits into from
Sep 18, 2020
Merged

Feature/google api #10

merged 11 commits into from
Sep 18, 2020

Conversation

Jwilson1172
Copy link
Contributor

@Jwilson1172 Jwilson1172 commented Sep 17, 2020

Description

This PR adds an OOP GoogleAPI service to the application. It also configures the endpoints in submission.py to provide text transcriptions and safe search moderation functionality.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Change Status

  • Complete, tested, ready to review and merge
  • Complete, but not tested (may need new tests)
  • Incomplete/work-in-progress, PR is for discussion/feedback

Has This Been Tested

  • Yes
  • No
  • Not necessary

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings1
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • There are no merge conflicts

1 - The class that is implemented in project/app/utils/google_api.py will throw an error and stop execution if no Google Credentials are provided in the environment variable GOOGLE_CREDS. the contents of this variable are the JSON in the service_account.json credential file that is downloaded from the Google Cloud Dashboard.

Copy link
Contributor

@lorischl-otter lorischl-otter left a comment

Choose a reason for hiding this comment

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

Looks great overall! Just added in a few suggestions for changes & documentation updates. Also, I'm unclear on the Change Status vs Has This Been Tested boxes that are checked in the PR template. Could you change that so it's not a conflicting message of "Complete, but not tested" and "Yes" it has been tested? Thanks!

project/app/api/submission.py Outdated Show resolved Hide resolved
project/app/api/submission.py Outdated Show resolved Hide resolved
project/app/utils/google_api.py Show resolved Hide resolved
@Jwilson1172
Copy link
Contributor Author

I also fixed the PR Description, I always assumed that one of the tested prompts was specific to unit-testing and one was specific to testing the functionality locally.

Copy link
Contributor

@schase15 schase15 left a comment

Choose a reason for hiding this comment

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

Looks great so far! I appreciate the thoroughness of the docstrings.

Copy link
Contributor

@lorischl-otter lorischl-otter left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

Copy link
Contributor

@lorischl-otter lorischl-otter left a comment

Choose a reason for hiding this comment

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

Thanks for the changes!

@lorischl-otter lorischl-otter merged commit f090df2 into main Sep 18, 2020
@Jwilson1172 Jwilson1172 mentioned this pull request Oct 6, 2020
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants