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

chore(texttospeech): modernize quickstart for texttospeech #3726

Merged
merged 15 commits into from
Aug 11, 2024

Conversation

iennae
Copy link
Contributor

@iennae iennae commented Jun 28, 2024

Description

Fixes #3504

(draft, still requires test update, region tags not added yet intentionally)

Note: Before submitting a pull request, please open an issue for discussion if you are not associated with Google.

Checklist

  • I have followed guidelines from CONTRIBUTING.MD and Samples Style Guide
  • Tests pass: npm test (see Testing)
  • Lint pass: npm run lint (see Style)
  • This pull request is from a branch created directly off of GoogleCloudPlatform/nodejs-docs-samples. Not a fork.
  • Please merge this PR for me once it is approved

@iennae iennae requested review from a team as code owners June 28, 2024 03:07
@product-auto-label product-auto-label bot added api: texttospeech Issues related to the Text-to-Speech API. samples Issues that are directly related to samples. labels Jun 28, 2024
@iennae iennae marked this pull request as draft June 28, 2024 03:07
texttospeech/package.json Outdated Show resolved Hide resolved
texttospeech/quickstart.mjs Outdated Show resolved Hide resolved
texttospeech/quickstart.mjs Outdated Show resolved Hide resolved
texttospeech/test/quickstart.test.mjs Outdated Show resolved Hide resolved
@iennae iennae marked this pull request as ready for review July 12, 2024 17:19
Copy link
Collaborator

@grayside grayside left a comment

Choose a reason for hiding this comment

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

I found no blockers, but left several comments with opportunities for additional minor improvements. Offering them because I see the scope as "modernizing" the quickstart, which leaves the scope ambiguously larger than the referenced issue.

texttospeech/quickstart.mjs Outdated Show resolved Hide resolved
texttospeech/test/quickstart.test.mjs Outdated Show resolved Hide resolved
texttospeech/test/quickstart.test.mjs Show resolved Hide resolved
texttospeech/test/quickstart.test.mjs Outdated Show resolved Hide resolved
texttospeech/quickstart.mjs Outdated Show resolved Hide resolved
texttospeech/quickstart.mjs Outdated Show resolved Hide resolved
@subfuzion subfuzion self-assigned this Aug 8, 2024
@subfuzion subfuzion force-pushed the chore-modernize-quickstart-texttospeech branch from 05457b5 to 2a8560d Compare August 10, 2024 23:48
@subfuzion subfuzion marked this pull request as draft August 10, 2024 23:56
@subfuzion
Copy link
Contributor

Hi @iennae, @grayside - I'm going to help with this as part of clearing the backlog.

@subfuzion subfuzion force-pushed the chore-modernize-quickstart-texttospeech branch from 103660e to be61da8 Compare August 11, 2024 00:39
@subfuzion subfuzion force-pushed the chore-modernize-quickstart-texttospeech branch from be61da8 to 58b1c3d Compare August 11, 2024 00:42
@subfuzion subfuzion marked this pull request as ready for review August 11, 2024 00:44
@subfuzion
Copy link
Contributor

subfuzion commented Aug 11, 2024

@iennae / @grayside I went ahead and did a bunch of refactoring (feel free to undo with a git reset --hard @~1; git push -f). Still room for improvement, but makes things a bit more consistent and idiomatic. I did mention that we should standardize on Node's native support for testing back in early May, but for this PR I did remove chai for standard assert/strict. We should talk a bit more about swapping out test runners later. I'm going to slam this through as part of clearing the backlog.

@subfuzion subfuzion merged commit 620fb04 into main Aug 11, 2024
31 checks passed
@subfuzion subfuzion deleted the chore-modernize-quickstart-texttospeech branch August 11, 2024 01:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: texttospeech Issues related to the Text-to-Speech API. samples Issues that are directly related to samples.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

texttospeech/quickstart.js change the file to use fs/promises
3 participants