Skip to content

Add Vision Client - #2179

Merged
daspecster merged 2 commits into
googleapis:masterfrom
daspecster:vision-client
Aug 29, 2016
Merged

Add Vision Client#2179
daspecster merged 2 commits into
googleapis:masterfrom
daspecster:vision-client

Conversation

@daspecster

@daspecsterdaspecster commented Aug 24, 2016

Copy link
Copy Markdown
Contributor

Areas of focus:

This PR is a bit large, but it's mostly due to vision/fixtures.py which I would like your feedback on.

The image source passed into the client will eventually be able to be a raw string/byte stream of the image, a gs://bucket/image.jpg or a URL to be downloaded via httplib2.

VisionJSONEncoder is probably a topic of debate.

VisionRequest should probably be in it's own file or I suppose it's functionality could be absorbed by Client. I'm not sure the best path there.

LMKWYT!

@daspecsterdaspecster added the api: vision Issues related to the Cloud Vision API. label Aug 24, 2016
@googlebotgooglebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 24, 2016
@dhermes

dhermes commented Aug 24, 2016

Copy link
Copy Markdown
Contributor

WIthout even looking, a custom json encoder seems like big time overkil (and may even be a performance hit).

@daspecster

daspecster commented Aug 24, 2016

Copy link
Copy Markdown
ContributorAuthor

Ok, I'll see if I can do without the JSON encoder.

tseaver
tseaver reviewed Aug 25, 2016
View reviewed changes
Comment threadgcloud/vision/test_client.py
PROJECT = 'PROJECT'
IMAGE_SOURCE = 'gs://some/image.jpg'
IMAGE_CONTENT = '/9j/4QNURXhpZgAASUkq'
B64_IMAGE_CONTENT = base64.b64encode(IMAGE_CONTENT)

This comment was marked as spam.

Sign in to view

@tseaver

tseaver commented Aug 25, 2016

Copy link
Copy Markdown
Contributor

gcloud.vision.test_image is missing.

tseaver
tseaver reviewed Aug 25, 2016
View reviewed changes
Comment threadgcloud/vision/test_client.py Outdated
return self._getTargetClass()(*args, **kw)

def test_make_vision_request(self):
IMAGE_CONTENT = '/9j/4QNURXhpZgAASUkq'

This comment was marked as spam.

Sign in to view

@daspecster

daspecster commented Aug 25, 2016

Copy link
Copy Markdown
ContributorAuthor

@tseaver, should I try and leave out image.py and try and test the client.py code with all just Mocks?

@tseaver

tseaver commented Aug 25, 2016

Copy link
Copy Markdown
Contributor

@daspecster the tests of the Client.image factory pretty much need to use the Image class: mocking it away doesn't win any clarity.

@daspecster
- Updated bytes usage
e55b036
- Add more docstrings and corrections
- Add gcs_uri support
- Remove VisionEncoder
@daspecster

daspecster commented Aug 29, 2016

Copy link
Copy Markdown
ContributorAuthor

@dhermes@tseaver I think I addressed your comments from before. As soon as this is merged I'll get the face detection PR together.

@tseaver

tseaver commented Aug 29, 2016

Copy link
Copy Markdown
Contributor

LGTM

@daspecster

daspecster commented Aug 29, 2016

Copy link
Copy Markdown
ContributorAuthor

Thanks @tseaver!

@dhermes did you have anything else?

@dhermes

dhermes commented Aug 29, 2016

Copy link
Copy Markdown
Contributor

You don't need two reviewers. Feel free to merge.

@daspecster
daspecster merged commit a678774 into googleapis:masterAug 29, 2016
@dhermesdhermes mentioned this pull request Sep 19, 2016
Merged
@daspecster
daspecster deleted the vision-client branch January 24, 2017 15:19
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@tseavertseaver

Labels

api: visionIssues related to the Cloud Vision API.cla: yesThis human has signed the Contributor License Agreement.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@daspecster@dhermes@tseaver@googlebot