Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Implement Document factory constructors on language client #2164

Merged
merged 5 commits into from
Aug 23, 2016
Merged

Implement Document factory constructors on language client #2164

merged 5 commits into from
Aug 23, 2016

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Aug 22, 2016 •
edited
Loading

NOTE: Has #2162 as diffbase.

@tseaver and @jgeewax this PR (namely the last 4 commits) make the discussion in #2062 / earlier today more concrete. Is this what you had in mind?

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Aug 22, 2016
@dhermes dhermes force-pushed the language-impl-2 branch 2 times, most recently from 9267a85 to 009833b Compare August 22, 2016 22:22
tseaver
tseaver reviewed Aug 22, 2016
View reviewed changes
gcloud/language/document.py

:type gcs_url: str
:param gcs_url: (Optional) The URL of the Google Cloud Storage object
holding the content.

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@tseaver
Copy link
Contributor

tseaver commented Aug 22, 2016 •
edited
Loading

I thought document_from_url was less useful than document_from_blob -- maybe I misunderstood? Again, I see you outpace me. :)

@dhermes dhermes force-pushed the language-impl-2 branch 3 times, most recently from ebb6c83 to a0ef4c9 Compare August 23, 2016 01:35
@tseaver
Copy link
Contributor

tseaver commented Aug 23, 2016

@dhermes To fix the json-docs error, add language to the mapping in scripts/generate_json_docs.py:main.

@dhermes
Copy link
Contributor Author

dhermes commented Aug 23, 2016

@tseaver Any issues? Any other factories needed?

tseaver
tseaver reviewed Aug 23, 2016
View reviewed changes
docs/language-usage.rst
>>> document = client.document_from_blob(bucket='my-text-bucket',
... blob='sentiment-me.txt')
>>> document = client.document_from_blob('my-text-bucket',
... 'sentiment-me.txt')

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@dhermes dhermes mentioned this pull request Aug 23, 2016
Merged
@dhermes
Copy link
Contributor Author

dhermes commented Aug 23, 2016

@tseaver Where do we stand on getting this merged? Do we want the document_from_blob() constructor? Should it behave differently? Should I punt on those factories for another PR?

@tseaver
Copy link
Contributor

tseaver commented Aug 23, 2016

I guess leave document_from_blob as is for now, and consider using the Blob class is an enhancement like #2127.

@dhermes
Copy link
Contributor Author

dhermes commented Aug 23, 2016

Filed #2173

@dhermes
Copy link
Contributor Author

dhermes commented Aug 23, 2016

@tseaver LGTY otherwise?

@tseaver
Copy link
Contributor

tseaver commented Aug 23, 2016

LGTM

@dhermes dhermes merged commit 9b3a082 into googleapis:master Aug 23, 2016
@dhermes dhermes deleted the language-impl-2 branch August 23, 2016 20:12
@dhermes dhermes mentioned this pull request Sep 19, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@tseaver tseaver

Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@dhermes @tseaver @jgeewax @daspecster @googlebot