Uh oh!
There was an error while loading. Please reload this page.
Update Translate samples to establish canonical template - #1734
Conversation
theacodes
commented
Oct 5, 2018
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): |
There was a problem hiding this comment.
Use a u prefix on text and it'll always be the Unicode type.
| translate_client = translate.Client() | ||
| text = 'Hello world' |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
What is NMT? Can this have a comment describing the placeholder variable?
Just want to also add my -1 to the format being proposed as well, for the record |
engelke
commented
Nov 20, 2018
Please address comments or close this PR. Thanks. |
engelke
commented
Dec 7, 2018
Comments have not been addressed in two months. Reopen if needed. |
chore: Add README for running zonal buckets samples
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.