-
Notifications
You must be signed in to change notification settings - Fork 45
Conversation
@@ -0,0 +1,134 @@ | |||
# 2.0.0 Migration Guide |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@software-dov @telpirion Please review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a few questions
UPGRADING.md
Outdated
In the 2.0.0 release, all methods have a single positional argument `request`. Method docstrings indicate whether an argument is | ||
required or optional. | ||
|
||
Some methods have additional keyword only arguments. The available parameters depend on the [`google.api.method_signature` annotation](https://github.com/googleapis/googleapis/blob/master/google/cloud/texttospeech/v1/cloud_tts.proto#L53) specified by the API producer. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be good to emphasize that the request
param and any kword only flattenings are mutually exclusive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added
["google/cloud/**/client.py", "google/cloud/**/cloud_tts.py"], | ||
"((en)|(no)|(nb)(cmn)|(yue))-\*", | ||
"\g<1>-\*", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure I understand this part. Is this indicative of an error in the template, bad escaping on the part of the generator, or just garbage input and garbage sphinx?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sphinx/RST interprets the *
as emphasis. I'd expect the generator to escape it? The proto comments look valid to me
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just a couple of questions.
Also, I would add the output.mp3 file to the .gitignore.
|
||
> **WARNING**: Breaking change | ||
|
||
Methods expect request objects. We provide a script that will convert most common use cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this script included in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes: scripts/fixup_keywords.py
is published with the library.
If you install the library it will be available on the command line. https://python-packaging.readthedocs.io/en/latest/command-line-scripts.html#the-scripts-keyword-argument
|
||
client = texttospeech.TextToSpeechClient() | ||
|
||
voices = client.list_voices(request={"language_code": "no"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a significant change in behavior. You might want to updated the docs too, at least a release note.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Will follow up with the TW.
## Enums and Types | ||
|
||
|
||
> **WARNING**: Breaking change |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above: consider updating the documentation on cloud.google.com for all breaking changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Formatting issues have been fixed. @telpirion PTAL
|
||
> **WARNING**: Breaking change | ||
|
||
Methods expect request objects. We provide a script that will convert most common use cases. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes: scripts/fixup_keywords.py
is published with the library.
If you install the library it will be available on the command line. https://python-packaging.readthedocs.io/en/latest/command-line-scripts.html#the-scripts-keyword-argument
|
||
client = texttospeech.TextToSpeechClient() | ||
|
||
voices = client.list_voices(request={"language_code": "no"}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Will follow up with the TW.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for these formatting changes! LGTM.
TODO: