Uh oh!
There was an error while loading. Please reload this page.
Vision a11 - adding crop hints and web annotation - #671
Conversation
tswast
left a comment
There was a problem hiding this comment.
Some general comments. I assume @jonparrott will have others.
| @@ -0,0 +1,97 @@ | |||
| #!/usr/bin/env python | |||
| # Copyright 2015 Google Inc. All Rights Reserved. | |||
There was a problem hiding this comment.
Also, I don't think the header on go/releasing/preparing includes "All Rights Reserved" anymore.
| discoveryServiceUrl=DISCOVERY_URL) | ||
| def crop_hint(photo_file): |
There was a problem hiding this comment.
photo_file makes me think this is a file object. Prefer photo_path.
(Update here and in the other functions)
@jonparrott The tests are successful locally. Any ideas why one of the three fails? It looks like the response is not as expected, but I can't tell why. |
theacodes
commented
Nov 21, 2016
@puneith the response list is empty: |
| import argparse | ||
| import base64 | ||
| import httplib2 |
There was a problem hiding this comment.
httplib2 is a third-party import.
| from googleapiclient import discovery | ||
| from oauth2client.client import GoogleCredentials | ||
| DISCOVERY_URL = ('https://vision.googleapis.com/$discovery/rest?' |
There was a problem hiding this comment.
Break after opening (, please.
| credentials = GoogleCredentials.get_application_default() | ||
| scoped_credentials = credentials.create_scoped( | ||
| ['https://www.googleapis.com/auth/cloud-platform']) | ||
| http = httplib2.Http() |
There was a problem hiding this comment.
I know I ask this constantly, but is this needed because of alpha?
| ['https://www.googleapis.com/auth/cloud-platform']) | ||
| http = httplib2.Http() | ||
| scoped_credentials.authorize(http) | ||
| return discovery.build('vision', 'v1', |
There was a problem hiding this comment.
Break after opening (, please.
| with open(photo_path, 'rb') as image: | ||
| image_content = base64.b64encode(image.read()) | ||
| service_request = service.images().annotate(body={ |
There was a problem hiding this comment.
Your with statement covers too many things. It only needs to cover the image.read call.
| service = get_service() | ||
| with open(photo_path, 'rb') as image: | ||
| image_content = base64.b64encode(image.read()) |
There was a problem hiding this comment.
Same comment here on the with statement.
| if args.command == 'crop_hint': | ||
| response = crop_hint(args.image_path) | ||
| print(json.dumps(response, indent=2)) |
There was a problem hiding this comment.
Prefer printing the result in the sample functions themselves.
There was a problem hiding this comment.
I prefer printing outside because then I can test easily. Otherwise I have to use capsys and parse the output again.
There was a problem hiding this comment.
Our samples should not be structured around our tests, it should be the other way around. Please be consistent with the other samples.
There was a problem hiding this comment.
Actually tests are a good way to find if the code is written properly. In this case testing become bit tedious if you print in the function vs the other option.
There was a problem hiding this comment.
We can have a separate conversation if you want to propose a change to the Python sample style, but for now, can you please be consistent?
| crop_hint_annotation = crop_hint['cropHintsAnnotation']['cropHints'][0] | ||
| confidence = crop_hint_annotation['confidence'] | ||
| assert confidence > 0.5 |
There was a problem hiding this comment.
Try assert 0.5 < confidence < 0.9
puneith
commented
Nov 21, 2016
@gguuss fyi |
theacodes
commented
Nov 21, 2016
@tswast can you approve changes? |
puneith
commented
Nov 22, 2016
@jonparrott Now that this is finally green can you please merge. |
puneith
commented
Nov 22, 2016
@dpebot Do I have powers to ask you to merge? |
* samples: delete unspecified sample * fixed lint issue * updated readme
@jonparrott@tswast Please review. Alpha 1.1 changes.