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.

Sign up for GitHub

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

Jump to bottom

Added @example tags to src/instance.js #222

Merged
merged 8 commits into from
Jul 20, 2018
Merged

Added @example tags to src/instance.js #222

merged 8 commits into from
Jul 20, 2018

Conversation

vijay-qlogic
Copy link
Contributor

@vijay-qlogic vijay-qlogic commented Jun 27, 2018 •
edited
Loading

Fixes #157 for Instance.js (it's a good idea to open an issue first for discussion)

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 27, 2018
@vijay-qlogic vijay-qlogic force-pushed the tag-example-comments branch from 7c8b1c9 to e80dfbd Compare June 27, 2018 06:03
@codecov
Copy link

codecov bot commented Jun 27, 2018 •
edited
Loading

Codecov Report

Merging #222 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #222   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines        1266   1266           
=====================================
  Hits         1266   1266
Impacted Files Coverage Δ
src/instance.js 100% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a292b54...2108cac. Read the comment docs.

@ghost ghost assigned JustinBeckwith Jun 27, 2018
@vijay-qlogic vijay-qlogic force-pushed the tag-example-comments branch from ccebaa3 to d9a7923 Compare June 28, 2018 03:48
@vijay-qlogic vijay-qlogic changed the title Added @example tags Added @example tags to src/instance.js Jun 28, 2018
@ghost ghost assigned sduskis Jun 28, 2018
@vijay-qlogic vijay-qlogic force-pushed the tag-example-comments branch from 34f55a0 to cbf4516 Compare June 29, 2018 15:23
@sduskis
Copy link
Contributor

sduskis commented Jun 29, 2018

@jabubake: Do we need tests for these snippets?

@jabubake
Copy link
Contributor

@sduskis : Yes, ensuring these are tested will help maintain them.

Since these snippets are for the API methods, one option to minimize the number of tests is to ensure that the integration tests for the library leverage these snippets as test cases.

@stephenplusplus
Copy link
Contributor

Samples actually have their own testing files and conventions: https://github.com/googleapis/nodejs-bigtable/tree/master/samples. See the system-test directory.

There is already a file, instances.js. Maybe these new tests should be moved into that file, then tested from system-test/instances.test.js?

@jabubake
Copy link
Contributor

@stephenplusplus I agree there is overlap here : in this case, samples/instance.js provides a clean way to run through basic use cases in a single format. The snippets are intended to focus on the the different ways of calling the API w/ promise, callback etc. This is a little unique to node.js, so i'm fine with the additional source file here vs cluttering samples/instance.js : we can maybe remove the cli from the document-snippets/instance.js and just have tests written against it, so that developers do not run into weird states running the CLI commands out of order.

@vijay-qlogic vijay-qlogic force-pushed the tag-example-comments branch 2 times, most recently from 7e8848b to a225c02 Compare July 4, 2018 17:51
sduskis
sduskis suggested changes Jul 6, 2018
View reviewed changes
samples/document-snippets/instance.js Outdated
const Bigtable = require('@google-cloud/bigtable');
const bigtableClient = new Bigtable();

function newInstance(instanceId) {

This comment was marked as spam.

Sign in to view

samples/document-snippets/instance.js Outdated
// let operations = result[1];
// let apiResponse = result[2];

console.log(`Created Instance: ${newInstance.name}`);

This comment was marked as spam.

Sign in to view

samples/document-snippets/instance.js Outdated
const newCluster = result[0];
// const operations = result[1];
// const apiResponse = result[2];
console.log(`Cluster created: ${newCluster.name}`);

This comment was marked as spam.

Sign in to view

samples/document-snippets/instance.js Outdated
// [END bigtable_create_instance]
}

function newCluster(clusterId, instanceId) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

samples/document-snippets/instance.js Outdated
// [END bigtable_create_cluster]
}

function newAppProfile(appProfileId, clusterId, instanceId) {

This comment was marked as spam.

Sign in to view

samples/document-snippets/instance.js Outdated
function getMetaData(instanceId) {
const instance = bigtableClient.instance(instanceId);

// [START bigtable_get_imeta]

This comment was marked as spam.

Sign in to view

samples/document-snippets/instance.js Outdated
console.log(`Tables:`);
let tables = result[0];
tables.forEach(t => {
console.log(t.name);

This comment was marked as spam.

Sign in to view

samples/document-snippets/instance.js Outdated
// [END bigtable_get_tables]
}

function setMetaData(instanceId) {

This comment was marked as spam.

Sign in to view

This comment was marked as spam.

Sign in to view

@sduskis
Copy link
Contributor

sduskis commented Jul 9, 2018

@vijay-qlogic: can you please update this?

@vijay-qlogic vijay-qlogic force-pushed the tag-example-comments branch from 8b64377 to 6c11f2a Compare July 10, 2018 04:43
@vijay-qlogic vijay-qlogic force-pushed the tag-example-comments branch from 6c11f2a to 174f67c Compare July 11, 2018 04:25
@sduskis
Copy link
Contributor

sduskis commented Jul 13, 2018 •
edited
Loading

@stephenplusplus, do you know how to generate API documentation? That's important in testing this PR. Specifically, how can we generate the API docs that pull in examples from other files:

   * @example <caption>include:samples/document-snippets/instance.js</caption>
   * region_tag:bigtable_create_cluster

@stephenplusplus
Copy link
Contributor

That's all done behind the scenes from me. @jmdobry can probably help.

@vijay-qlogic
Copy link
Contributor Author

I searched for generating API docs that pull in examples from other files, but no where in JSDoc guide found region_tag being used to for same.
@jmdobry can you please help here.

@jmdobry
Copy link
Contributor

jmdobry commented Jul 16, 2018

If your changes were on a branch of the nodejs-bigtable repo itself, I could stage the API ref docs to test their use of region_tag. I don't have a way to test when the changes are on a fork.

@sduskis
Copy link
Contributor

sduskis commented Jul 18, 2018

@jmdobry, can you potentially use vijay-qlogic:tag-example-comments as the branch?

@vijay-qlogic
Copy link
Contributor Author

vijay-qlogic commented Jul 18, 2018 •
edited
Loading

@jmdobry I have referred nodejs-spanner to include region_tag, But I could not generate expected API documentation in nodejs-spanner repository as well.

I searched for region_tag on http://usejsdoc.org/ as well but didn't got any clue.

@sduskis sduskis changed the base branch from master to docs_test July 20, 2018 15:36
@sduskis sduskis merged commit df2b0c6 into googleapis:docs_test Jul 20, 2018
@sduskis
Copy link
Contributor

sduskis commented Jul 20, 2018

@jmdobry, I created a new branch on nodejs-bigtable called docs-test. Can you please try it out?

@jmdobry
Copy link
Contributor

jmdobry commented Jul 20, 2018

I tested generating and staging the docs using your docs_test branch, looks good.

@vijay-qlogic
Copy link
Contributor Author

vijay-qlogic commented Jul 23, 2018 via email

@jmdobry
Copy link
Contributor

jmdobry commented Jul 23, 2018

Has something changed since I last verified it?

@sduskis
Copy link
Contributor

sduskis commented Jul 23, 2018

@vijay-qlogic, this was verified.

sduskis added a commit that referenced this pull request Jul 24, 2018
@sduskis
Added @example tags to src/instance.js (#222) (#252)
4bfd84e 
* Added @example tags

* added result log

* moved instance-snippets to document-snippets/instance

* reference updated

* added code-review change requests

* Added Test Cases to check for NO-Error in instance-snippets
@snyk-bot snyk-bot mentioned this pull request Mar 4, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sduskis sduskis sduskis requested changes

Assignees

@sduskis sduskis

@JustinBeckwith JustinBeckwith

Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@vijay-qlogic @sduskis @jabubake @stephenplusplus @jmdobry @JustinBeckwith @googlebot