Add initial Sentiment Analysis tutorial to Natural Language samples #524

manshreck · 2016-09-14T21:48:11Z

This sample will be used within the Sentiment Analysis Tutorial at:

https://cloud.google.com/natural-language/docs/sentiment-tutorial

This pull does not yet include [SECTION] tags, which I will add subsequently.

googlebot · 2016-09-14T21:48:13Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

jerjou · 2016-09-14T21:50:34Z

language/sentiment/README.txt

+## Download the Code
+
+```
+git clone sso://python-dev-samples/language/sentiment/


git clone https://github.com/GoogleCloudPlatform/python-docs-samples.git

jerjou · 2016-09-14T21:50:49Z

language/sentiment/README.txt

@@ -0,0 +1,27 @@
+# Introduction


The file should have an .md extension

jerjou · 2016-09-14T21:51:25Z

language/sentiment/README.txt

+
+This sample contains the code referenced in the 
+[Sentiment Analysis Tutorial](http://cloud.google.com/natural-language/docs/sentiment-tutorial)
+within the Natural Language API Documentation. A full walkthrough of this sample


First mention of the product should use the entire (verbose) name

jerjou · 2016-09-14T21:52:34Z

language/sentiment/README.txt

+git clone sso://python-dev-samples/language/sentiment/
+```
+
+### Usage


Should probably add a section for creating a virtualenv and installing the requirements. example

Is this really necessary? I don't see that in a lot of other samples, and I think it will add a lot of implementation detail that's unnecessary. For complex samples, using virtualenv and a requirements.txt file is good practice, but I don't quite agree it should be always the case.

Yes, every sample should use a virtualenv. Not using a virtualenv is like going out in the rain without an umbrella.

Can you show me a README that you wish to use as the canonical README for that purpose?

http://go/python-samples-guide#canonical-sample has apparently been factored away, but you can check out python-docs-samples/storage for its README

Also added requirements.txt

jerjou · 2016-09-14T21:53:27Z

language/sentiment/mixed.txt

@@ -0,0 +1,20 @@
+I really wanted to love 'Bladerunner' but ultimately I couldn't get


Can you rename the sentiment dir to resources, for consistency with the other samples

The "sentiment" dir refers to this tutorial, and I'd like to keep it named for what it is. Did you mean move the test files into a subdirectory: "sentiment/resources"? I can do that.

No - for all the other python samples, files used by the code for testing, etc, are in a directory named resources. Unless there's a good reason to deviate, having consistency between our samples is valuable. Can you update your tutorial to say "resources" instead of "sentiment"?

You're missing my point. All of these files are already in a "language/sentiment" directory. I'm not renaming that to "language/resources." If you want me to put these text files into a subdirectory so that they are in a "language/sentiment/resources" directory, that I can do.

Yes, language/sentiment/resources would be preferable.

jerjou · 2016-09-14T21:53:53Z

language/sentiment/sentiment_analysis.py

+    http = httplib2.Http()
+    credentials.authorize(http)
+
+    service = discovery.build('language', 'v1beta1', http=http)


You can just do:

credentials = GoogleCredentials.get_application_default() service = discovery.build('language', 'v1beta1', credentials=credentials)

The python client lib will take care of adding the right scope, and authorizing the http object.

I prefer the former way even though it is more steps, because it emphasizes that you first get credentials, and then authorize them explicitly via http (and that's how I document it in the tutorial).

Please use the idiomatic way that @jerjou suggested, there's absolutely no reason to expose the http client to the user, and we're actively trying to remove httplib2. The less samples I have to clean up after that, the better.

jerjou · 2016-09-14T21:54:04Z

language/sentiment/sentiment_analysis.py

+    try:
+        polarity = response['documentSentiment']['polarity']
+        magnitude = response['documentSentiment']['magnitude']
+    except KeyError:


Just let the KeyError propagate up. Error handling doesn't really provide much educational value IMO..

Will do, but there are competing philosophies about this. I, for one, don't like to see typical Python stack traces for known error cases, even if they aren't handled.

Yeah - I think it comes down to trading off teaching about best practices, vs keeping the signal / noise ratio high (where 'signal' is 'stuff that's relevant to the api I'm trying to learn about'). My feeling is that sample code should be as focused as is prudent on the specific thing you're trying to teach, so the user doesn't have to wade through boilerplate to get at the point. More general best practices can be covered in other venues, where they can be given the exposition they deserve.

Ackknowledged.

jerjou · 2016-09-14T21:54:15Z

language/sentiment/sentiment_analysis.py

+        polarity = response['documentSentiment']['polarity']
+        magnitude = response['documentSentiment']['magnitude']
+    except KeyError:
+        print("The response did not contain the expected fields.")


style: prefer single quotes over double quotes

jerjou · 2016-09-14T21:54:24Z

language/sentiment/sentiment_analysis.py

+    except KeyError:
+        print("The response did not contain the expected fields.")
+    print('Sentiment: polarity of %s with magnitude of %s'
+          % (polarity, magnitude))


Prefer using the 'string {}'.format(arg) syntax

jerjou · 2016-09-14T21:54:34Z

language/sentiment/sentiment_analysis.py

+
+
+if __name__ == '__main__':
+    parser = argparse.ArgumentParser()


Add a module-level docstring and set that as the description. See http://go/python-sample-guide#argparse-section

googlebot · 2016-09-15T22:55:43Z

CLAs look good, thanks!

googlebot · 2016-09-15T23:29:32Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

jerjou · 2016-09-16T00:08:26Z

language/sentiment/README.md

+
+```
+$ gcloud init
+```


Actually, I think this won't work. Does gcloud init set up a service account for you? IIUC, without setting up a service account, this will attempt to use the gcloud auth, which doesn't have the api enabled.

jerjou · 2016-09-16T00:44:28Z

language/sentiment/README.md

+
+## Prerequisites
+
+1. Install the [Google Cloud SDK](https://cloud.google.com/sdk/), including the [gcloud tool](https://cloud.google.com/sdk/gcloud/), and [gcloud app component](https://cloud.google.com/sdk/gcloud-app).


This step isn't necessary anymore

jerjou

Looks good, after removing step 1

up a project entails.

jerjou · 2016-09-16T19:52:10Z

@dpebot could you kindly merge this pull request once travis has given its approval? Many thanks! Cheerio and all the best in all your endeavors!

theacodes · 2016-09-16T19:54:31Z

@jerjou I'm gonna have to add that to his trigger phrases.

theacodes · 2016-09-16T19:54:34Z

@dpebot merge when travis passes

#triggered.

dpebot · 2016-09-16T19:54:35Z

Okay! I'll merge when all statuses are green.

…ples

jerjou · 2016-09-17T04:01:18Z

Still can't merge it :-(

theacodes · 2016-09-19T19:41:54Z

@jerjou I merged your other PR, is this one still needed?

jerjou · 2016-09-19T19:55:06Z

Yeah - this one can be closed.

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Sep 14, 2016

jerjou suggested changes Sep 14, 2016

View reviewed changes

manshreck removed the cla: no This human has *not* signed the Contributor License Agreement. label Sep 15, 2016

googlebot added the cla: no This human has *not* signed the Contributor License Agreement. label Sep 15, 2016

manshreck force-pushed the master branch from 1f74019 to e3265d7 Compare September 15, 2016 22:55

googlebot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Sep 15, 2016

Add sentiment analysis sample

cc396d0

theacodes force-pushed the master branch from e3265d7 to cc396d0 Compare September 15, 2016 23:29

googlebot added cla: no This human has *not* signed the Contributor License Agreement. and removed cla: yes This human has signed the Contributor License Agreement. labels Sep 15, 2016

Move sample review files into resources directory

3e90d6f

jerjou approved these changes Sep 16, 2016

View reviewed changes

jerjou suggested changes Sep 16, 2016

View reviewed changes

manshreck added 2 commits September 15, 2016 17:09

Remove blank line from end of file

6d51ccc

Update set up instructions to point to getting started guide

148f5be

jerjou reviewed Sep 16, 2016

View reviewed changes

jerjou approved these changes Sep 16, 2016

View reviewed changes

Update README to remove need to set up gcloud. Itemize what setting

b0c69b4

up a project entails.

dpebot added the automerge Merge the pull request once unit tests and other checks pass. label Sep 16, 2016

manshreck added 2 commits September 16, 2016 14:10

Update NL README to link to Sentiment tutorial code

80bf146

Merge branch 'master' of https://github.com/manshreck/python-docs-sam…

8e645c2

…ples

jerjou approved these changes Sep 16, 2016

View reviewed changes

jerjou approved these changes Sep 17, 2016

View reviewed changes

jerjou mentioned this pull request Sep 17, 2016

Add sentiment analysis sample #533

Merged

elibixby assigned jerjou Sep 17, 2016

Coerce number types before comparison

f11678e

theacodes closed this Sep 19, 2016

		@@ -0,0 +1,20 @@
		I really wanted to love 'Bladerunner' but ultimately I couldn't get



		if __name__ == '__main__':
		parser = argparse.ArgumentParser()


		## Prerequisites

		1. Install the [Google Cloud SDK](https://cloud.google.com/sdk/), including the [gcloud tool](https://cloud.google.com/sdk/gcloud/), and [gcloud app component](https://cloud.google.com/sdk/gcloud-app).

Add initial Sentiment Analysis tutorial to Natural Language samples #524

Add initial Sentiment Analysis tutorial to Natural Language samples #524

Conversation

manshreck commented Sep 14, 2016

googlebot commented Sep 14, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

googlebot commented Sep 15, 2016

googlebot commented Sep 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jerjou left a comment

Choose a reason for hiding this comment

jerjou commented Sep 16, 2016

theacodes commented Sep 16, 2016

theacodes commented Sep 16, 2016

dpebot commented Sep 16, 2016

jerjou commented Sep 17, 2016

theacodes commented Sep 19, 2016

jerjou commented Sep 19, 2016