-
Notifications
You must be signed in to change notification settings - Fork 44
feat!: regenerate with microgenerator #30
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
software-dov
left a comment
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
telpirion
left a comment
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.
busunkim96
left a comment
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.
telpirion
left a comment
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: