Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Update Translate samples to establish canonical template#1734

Closed
alixhami wants to merge 2 commits into
masterGoogleCloudPlatform/python-docs-samples:masterfrom
translate-canonicalGoogleCloudPlatform/python-docs-samples:translate-canonicalCopy head branch name to clipboard
Closed

Update Translate samples to establish canonical template#1734
alixhami wants to merge 2 commits into
masterGoogleCloudPlatform/python-docs-samples:masterfrom
translate-canonicalGoogleCloudPlatform/python-docs-samples:translate-canonicalCopy head branch name to clipboard

Conversation

@alixhami

@alixhami alixhami commented Oct 4, 2018

Copy link
Copy Markdown
Contributor

Follows new code sample rubric. Please review for compliance with the new code sample rubric and express any language-specific considerations that need to be addressed for this to be the canonical sample.

@alixhami alixhami added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Oct 4, 2018
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Oct 4, 2018
@theacodes

Copy link
Copy Markdown
Contributor

I really, really do not like the idea of our snippets being embedded in tests. Our tests are complex to run. I pushed back on this explicitly when this change was proposed and asked for user research before implementing this.

I'm am -1 on the structure, but otherwise fine with the contents.

text = 'Hello world'
target = 'is' # Target must be an ISO 639-1 language code.
model = translate.NMT
if isinstance(text, six.binary_type):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Use a u prefix on text and it'll always be the Unicode type.


translate_client = translate.Client()

text = 'Hello world'

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I just reviewed another PR that does the same update to a new canonical template, but the placeholder variables looked different there. There was a # TODO(developer) comment. Should that be added above these placeholder variables?

Also, should this be:

text = 'Your text to analyze, e.g. Hello world'


text = 'Hello world'
target = 'is' # Target must be an ISO 639-1 language code.
model = translate.NMT

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What is NMT? Can this have a comment describing the placeholder variable?

@beccasaurus beccasaurus self-requested a review October 5, 2018 23:05
@beccasaurus

beccasaurus commented Oct 5, 2018

Copy link
Copy Markdown
Contributor

Just want to also add my -1 to the format being proposed as well, for the record

@beccasaurus beccasaurus requested review from averikitsch and removed request for beccasaurus October 15, 2018 20:18
@engelke

engelke commented Nov 20, 2018

Copy link
Copy Markdown
Contributor

Please address comments or close this PR. Thanks.

@engelke

engelke commented Dec 7, 2018

Copy link
Copy Markdown
Contributor

Comments have not been addressed in two months. Reopen if needed.

@engelke engelke closed this Dec 7, 2018
@engelke engelke deleted the translate-canonical branch December 27, 2018 22:08
chalmerlowe pushed a commit that referenced this pull request Apr 7, 2026
chore: Add README for running zonal buckets samples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes This human has signed the Contributor License Agreement. do not merge Indicates a pull request not ready for merge, due to either quality or timing.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

Morty Proxy This is a proxified and sanitized view of the page, visit original site.