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.

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

language: re-work encoding property #2054

Merged
merged 2 commits into from
Mar 9, 2017

Conversation

stephenplusplus
Copy link
Contributor

@stephenplusplus stephenplusplus commented Mar 6, 2017

Fixes #2018

  • remove encoding parameter from the document. Add it as the optional parameter of methods like analyze
  • introduce default encoding -- I think the default encoding would be UTF16, since it's the encoding of a string. Might be worth switching the default encoding based on the type of the content, like UTF16 for string while UTF8 for buffer.
  • add aliases of encoding; 'string' for an alias of UTF16, and 'buffer' for UTF8, since many JS programmers are not aware of text encoding of JS string.
  • use a different word for the encoding option in the JS API surface; I guess users will easily misunderstand and wrongly set the parameters. NOTE I kept both, encoding and encodingType are allowed.

To Dos

@monattar could you please take a look?

#2018 explained that encodingType was not a value attached to a document. But, the "Document" object that we have in our API is just meant to cache content that will be used again for future analysis. So, since the "Document" holds the value, it seems like a fine place to set the encoding type (by the user or by us, using defaults). By not allowing users to set it at the time of Document creation, we are forcing them to set it each time they make a method call, even though the encodingType should be the same each time-- right?

@stephenplusplus stephenplusplus added the api: language Issues related to the Cloud Natural Language API API. label Mar 6, 2017
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 6, 2017
@@ -44,8 +44,6 @@ var prop = require('propprop');
* object to specify the encoding and/or language of the document, use this
* property to pass the inline content of the document or a Storage File
* object.
* @param {string} options.encoding - `UTF8`, `UTF16`, or `UTF32`. See
* [`EncodingType`](https://cloud.google.com/natural-language/reference/rest/v1/EncodingType).

This comment was marked as spam.

This comment was marked as spam.

Copy link

@monattar monattar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the changes. Added a minor comment.

@stephenplusplus
Copy link
Contributor Author

@monattar thanks for the review!

@callmehiphop PTAL.

@stephenplusplus stephenplusplus merged commit eb20709 into googleapis:master Mar 9, 2017
@coveralls
Copy link

Coverage Status

Changes Unknown when pulling 94cb07a on stephenplusplus:spp--2018 into ** on GoogleCloudPlatform:master**.

sofisl pushed a commit that referenced this pull request Oct 11, 2022
sofisl pushed a commit that referenced this pull request Oct 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: language Issues related to the Cloud Natural Language API API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants