Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

feat(samples): Adding code samples and tests for them #560

Merged
merged 10 commits into from
Jun 3, 2021
Merged

feat(samples): Adding code samples and tests for them #560

merged 10 commits into from
Jun 3, 2021

Conversation

FrodoTheTrue
Copy link
Contributor

@FrodoTheTrue FrodoTheTrue commented May 24, 2021 •
edited
Loading

Thank you for opening a Pull Request! Before submitting your PR, there are a few things you can do to make sure it goes smoothly:

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

Fixes #<issue_number_goes_here> 🦕

@FrodoTheTrue FrodoTheTrue requested a review from a team as a code owner May 24, 2021 15:47
@google-cla google-cla bot added the cla: yes This human has signed the Contributor License Agreement. label May 24, 2021
@product-auto-label product-auto-label bot added api: compute Issues related to the googleapis/nodejs-compute API. samples Issues that are directly related to samples. labels May 24, 2021
@snippet-bot
Copy link

snippet-bot bot commented May 24, 2021 •
edited
Loading

Here is the summary of changes.

You are about to add 7 region tags.
You are about to delete 1 region tag.

This comment is generated by snippet-bot.
If you find problems with this result, please file an issue at:
https://github.com/googleapis/repo-automation-bots/issues.
To update this comment, add snippet-bot:force-run label or use the checkbox below:

  • Refresh this comment

@FrodoTheTrue FrodoTheTrue mentioned this pull request May 24, 2021
Closed
@FrodoTheTrue FrodoTheTrue requested a review from fhinkel May 26, 2021 07:30
@fhinkel
Copy link
Contributor

fhinkel commented May 27, 2021

Thanks for submitting more samples. Ideally each function should be in its own file, not inside quickstart.js.

fhinkel
fhinkel suggested changes May 27, 2021
View reviewed changes
Copy link
Contributor

@fhinkel fhinkel left a comment

Choose a reason for hiding this comment

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

Each function should be in its own file. With a non-async main function and the last line in the file should be main(...process.argv.slice(2));.

fhinkel
fhinkel reviewed May 27, 2021
View reviewed changes
samples/quickstart.js Outdated Show resolved Hide resolved
@FrodoTheTrue
Copy link
Contributor Author

@fhinkel could you please review again?

FrodoTheTrue
FrodoTheTrue commented Jun 1, 2021
View reviewed changes
samples/waitForOperation.js Outdated
* @param {string} projectId - ID or number of the project you want to use.
* @param {Operation} operation - Operation instance you want to wait.
*/
function main(projectId, operation) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am not sure how to test this file, cause we need to pass operation object from CLI params

Copy link
Contributor

Choose a reason for hiding this comment

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

Just pass the params needed to create an operation object and create it inside main.

@fhinkel fhinkel added the kokoro:run Add this label to force Kokoro to re-run the tests. label Jun 2, 2021
@fhinkel
Copy link
Contributor

fhinkel commented Jun 2, 2021

Can you fix the license header violations please.

fhinkel
fhinkel reviewed Jun 2, 2021
View reviewed changes
samples/mailjet.js Outdated

'use strict';

// [START send]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use proper product naming for the tag please. Thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its just copy of current files. should i change them to new one?
https://github.com/googleapis/nodejs-compute/blob/master/samples/mailjet.js

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to correct the old files as well. New files have to pass the license header and tag checks, otherwise we can't merge.

@fhinkel fhinkel merged commit 30cd27d into googleapis:diregapic Jun 3, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@fhinkel fhinkel fhinkel approved these changes

Assignees
No one assigned
Labels
api: compute Issues related to the googleapis/nodejs-compute API. cla: yes This human has signed the Contributor License Agreement. kokoro:run Add this label to force Kokoro to re-run the tests. samples Issues that are directly related to samples.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@FrodoTheTrue @fhinkel