-
Notifications
You must be signed in to change notification settings - Fork 6.4k
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
Adds snippets and tests for documentation tutorial. #729
Conversation
|
||
|
||
def authenticate(): | ||
'''Authenticates the client library using default application |
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.
Use double quotes for docstrings, please.
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.
Done
return service | ||
|
||
|
||
def getResponse(filename): |
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.
snake_case
for everything but class names, please.
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.
Done.
# [END import_libraries] | ||
|
||
|
||
def authenticate(): |
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.
We almost universally avoid having a separate function construct the client. Indirection can be confusing for users. This is two lines of code, would you mind just in-lining this in the relevant functions?
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.
On second thought since this is part of a cohesive tutorial and not snippets this can stay. But can you rename it to something like make_client
?
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.
Done. I ended up removing the separate function, it may actually make sense to flatten the whole sample into a single helper that accepts a filename.
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 would be for that.
# [START parsing_the_response] | ||
score = response['documentSentiment']['score'] | ||
magnitude = response['documentSentiment']['magnitude'] | ||
for i, sentence in enumerate(response['sentences']): |
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.
Nit: use n
when using enumerate.
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.
Done, thanks.
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.
Done.
def test_neutral(): | ||
result = tutorial.getResponse('reviews/bladerunner-neutral.txt') | ||
assert result['language'] == 'en' | ||
assert (result['documentSentiment']['score'] > -1 and |
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.
Python trick for you:
assert -1 < result['documentSentiment']['score'] < 1
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.
Nice! Done.
import argparse | ||
|
||
from googleapiclient import discovery | ||
|
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.
No empty line between these two.
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.
Done.
service = discovery.build('language', 'v1', credentials=credentials) | ||
# [END authenticating_to_the_api] | ||
# [START constructing_the_request] | ||
with open(filename, 'r') as review_file: |
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.
- Put an empty newline above control statements
- You didn't limit the scope of the with statement as I mentioned in the last round. :(
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.
Done.
# [START parsing_the_response] | ||
score = response['documentSentiment']['score'] | ||
magnitude = response['documentSentiment']['magnitude'] | ||
for n, sentence in enumerate(response['sentences']): |
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.
blank newline above control statements.
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.
Done.
sentence_sentiment = sentence['sentiment']['score'] | ||
print('Sentence {} has a sentiment score of {}'.format(n, | ||
sentence_sentiment)) | ||
print('Overall Sentiment: score of {} with magnitude of {}'.format( |
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.
Blank newline above this statement, please.
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.
Done.
|
||
|
||
# [START running_your_application] | ||
def main(filename): |
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.
You can remove this function 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.
Done.
Adds code to back up documentation. Note that on my machine, there are issues with generating the documentation so that still should be added before merging.