Add a Google Cloud Natural Language API sample. #164
Conversation
bshaffer
left a comment
bshaffer
left a comment
There was a problem hiding this comment.
This is all awesome. A handful of small changes is all.
| */ | ||
| function analyze_everything($text, $options = []) | ||
| { | ||
| $builder = new ServiceBuilder(); |
There was a problem hiding this comment.
We decided to stop using the ServiceBuilder in examples, and instead just instantiate the client directly, i.e.
$language = new NaturalLanguageClient();| * analyze_everything($projectId, 'Do you know the way to San Jose?'); | ||
| * ```. | ||
| * | ||
| * @param string $projectId The Google project ID. |
There was a problem hiding this comment.
Remove unused $projectId param from docblock/example
| { | ||
| $text = implode(" ", $input->getArgument('text')); | ||
| $result = analyze_entities($text); | ||
| print_r($result->info()); |
There was a problem hiding this comment.
In the other samples, we have the output take place inside the included functions. This is so that the copy/paste sample outputs something meaningful by default.
print_r is pretty generic, could we output something more structured? Something like:
printf('Name: %s' . PHP_EOL, $annotation['name']);
printf('Tag: %s' . PHP_EOL, $annotation['tag']);
printf('Sentiment: polarity of %s with magnitude of %s' . PHP_EOL,
$annotation['documentSentiment']['polarity'],
$annotation['documentSentiment']['magnitude']);There was a problem hiding this comment.
What if we did something like this for syntax:
print('TOKENS' . PHP_EOL);
$info = $result->info();
foreach ($info['tokens'] as $token) {
printf('%s - %s' . PHP_EOL, $token['text']['content'], $token['text']['partOfSpeech']);
}And something similar for entities?
There was a problem hiding this comment.
I feel a tug of war between good coding discipline and a good demonstration of the code. Coding discipline dictates that a function should return values, not print values. Command handlers should print values. In other samples, I think the functions were quite illustrative. But in this sample, the functions are only 3 lines with no branches. What do you think of just moving the code into the commands and deleting the functions? The unit tests that test the commands already 100% cover the functions.
Regarding the print_r, will PHP programmers be familiar with the print_r output? As a reader, I definitely want to see every annotation that language API can give me, and print_r shows me every detail, and exactly where to find it. Also, writing custom print statements means more code for us to maintain, and that print code would be unlikely to be copied and pasted into user code.
|
See the output from |
SurferJeffAtGoogle
force-pushed
the
cherrypick-language
branch
from
92f9ee5 to
7b46b37
Compare
bshaffer
left a comment
bshaffer
left a comment
There was a problem hiding this comment.
Linter fixes, but otherwise LGTM!
| } | ||
|
|
||
| if (isset($info['documentSentiment'])) | ||
| { |
There was a problem hiding this comment.
This will cause the linter to fail. They need to be cuddly!
|
|
||
| use Google\Cloud\NaturalLanguage\Annotation; | ||
|
|
||
| function print_annotation(Annotation $annotation) { |
There was a problem hiding this comment.
Curly brace here needs to be on its own line.
| print "entities:\n"; | ||
| foreach ($info['entities'] as $entity) { | ||
| print " " . $entity['name']; | ||
| if (isset($entity['metadata']['wikipedia_url'])) |
There was a problem hiding this comment.
Always use curly braces even for one-liners.
| function print_annotation(Annotation $annotation) { | ||
| $info = $annotation->info(); | ||
| if (isset($info['language'])) { | ||
| print "language: " . $info['language'] . "\n"; |
There was a problem hiding this comment.
We've been using single quotes for all strings, and PHP_EOL instead of \n
2 participants
No description provided.