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

add task list example application #28

Merged
merged 1 commit into from
Nov 16, 2015

Conversation

stephenplusplus
Copy link
Contributor

No description provided.

@JustinBeckwith
Copy link
Contributor

LGTM

@@ -4,6 +4,9 @@
"version": "0.0.1",
"private": true,
"license": "Apache Version 2.0",
"bin": {
Copy link
Member

Choose a reason for hiding this comment

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

Nobody is going to be installing this sample via npm, and thus no node_modules/.bin symlink will be created. Perhaps you meant this to be "scripts", as done here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

npm link instead of npm install will make it available. The instructions section above suggests that, since it will install the deps and link at the same time.

Copy link
Member

Choose a reason for hiding this comment

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

All of the samples in the repo should be consistent on how they instruct the user to run the sample. Do you think npm link and "bin" is better than how the storage/ samples does it? I'm leaning towards how the storage/ sample does it because it keeps everything local to the sample without affecting something elsewhere on the user's comp. What are your thoughts?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, probably not worth dealing with issues about the symlink failing due to permissions and such. It does allow a nicer interface, though:

$ datastore-tasks list

vs.

$ npm run datastore-tasks list

I'll switch this over to the second style.

Regarding the consistency note, do you prefer datastoreTasks to match authSample? Better yet, maybe just tasks?:

$ npm run tasks list

Copy link
Member

Choose a reason for hiding this comment

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

authSample follows the convention where the command for executing the sample has the same name as the sample file itself, e.g. npm run authSample because the file is named authSample.js. So here, the names should be consistent. If you want the command to be tasks, then you should rename the file to tasks.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated

jmdobry added a commit that referenced this pull request Nov 16, 2015
add task list example application
@jmdobry jmdobry merged commit 1be77f4 into GoogleCloudPlatform:master Nov 16, 2015
pattishin pushed a commit that referenced this pull request Nov 9, 2022
* feat: adds cleanup function for tests
pattishin pushed a commit that referenced this pull request Nov 9, 2022
* feat: adds cleanup function for tests
pattishin pushed a commit that referenced this pull request Nov 9, 2022
* feat: adds cleanup function for tests
NimJay pushed a commit that referenced this pull request Nov 11, 2022
* feat: adds cleanup function for tests
unforced pushed a commit that referenced this pull request Nov 17, 2022
* Release v0.1.1

* Update CHANGELOG.md
kweinmeister pushed a commit that referenced this pull request Nov 18, 2022
ahrarmonsur pushed a commit that referenced this pull request Nov 18, 2022
jsimonweb pushed a commit to jsimonweb/nodejs-docs-samples that referenced this pull request Dec 12, 2022
* chore: remove vendored dependencies

* chore: remove replace

* chore(ci): fix lint workflow

* chore: update alloydbconn version
irataxy added a commit that referenced this pull request Jan 18, 2023
* docs: add Live Stream samples and tests

* Add project number needed for samples test in kokoro configs.

* Remove project number requirements. Use project ID for function parameters

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
grayside pushed a commit that referenced this pull request Jan 18, 2023
* docs: add Live Stream samples and tests

* Add project number needed for samples test in kokoro configs.

* Remove project number requirements. Use project ID for function parameters

* 🦉 Updates from OwlBot post-processor

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

Co-authored-by: Owl Bot <gcf-owl-bot[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants