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

Storage ACL samples. #172

Merged
merged 2 commits into from
Aug 18, 2016
Merged

Storage ACL samples. #172

merged 2 commits into from
Aug 18, 2016

Conversation

jmdobry
Copy link
Member

@jmdobry jmdobry commented Aug 18, 2016

  • - Cloud Storage ACL samples
  • - Unit tests
  • - System tests
  • - Sample CLI
  • - storage/README.md updated

@jmdobry
Copy link
Member Author

jmdobry commented Aug 18, 2016

@ace-n Ready for review

__Usage:__ `node acl --help`

```
Commands:
Copy link
Contributor

Choose a reason for hiding this comment

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

Wishlist/Pipe dream: auto-generate these.

Copy link
Member Author

@jmdobry jmdobry Aug 18, 2016

Choose a reason for hiding this comment

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

You mean auto-generate all of README.md? The usage information was generated, I just ran node acl --help and pasted the output into README.md. I used to have to write all that by hand, now most of README.md is generated.

Copy link
Contributor

Choose a reason for hiding this comment

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

Generated in the "I don't have to even paste things in, I just run a script" sense.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think there's negligible work to be saved by scripting the copy paste. I mean, it could be scripted, but we'd need to devise a manifest.json file or something that could give the script meta information about the samples in the directory. Maybe it's not such a bad idea...

@codecov-io
Copy link

Current coverage is 87.09% (diff: 97.87%)

Merging #172 into master will increase coverage by 0.20%

@@             master       #172   diff @@
==========================================
  Files            56         57     +1   
  Lines          2486       2533    +47   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits           2160       2206    +46   
- Misses          326        327     +1   
  Partials          0          0          

Powered by Codecov. Last update e8e3e9d...dd1b552


sample.program.deleteAccessControl(options, callback);

assert(callback.calledOnce, 'callback called once');
Copy link
Contributor

Choose a reason for hiding this comment

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

What are the tradeoffs of this pattern vs. defining a callback function and putting the tests in the there? (And which should I be using in my tests?)

Copy link
Member Author

Choose a reason for hiding this comment

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

I would say the two main benefits are:

  1. Use of sinon.stub as the callback allows for additional assertions about the code execution (it gathers more info about how the callback is used)
  2. The test function now actually looks synchronous in addition to being synchronous.

@ace-n
Copy link
Contributor

ace-n commented Aug 18, 2016

LGTM other than a few minor nits.

@jmdobry jmdobry merged commit e4958b0 into master Aug 18, 2016
@jmdobry jmdobry deleted the permissions branch August 18, 2016 07:06
kweinmeister pushed a commit that referenced this pull request Nov 10, 2022
ace-n pushed a commit that referenced this pull request Nov 11, 2022
ace-n pushed a commit that referenced this pull request Nov 11, 2022
pattishin pushed a commit that referenced this pull request Nov 18, 2022
🤖 I have created a release \*beep\* \*boop\*
---
### [1.9.1](https://www.github.com/googleapis/nodejs-ai-platform/compare/v1.9.0...v1.9.1) (2021-07-21)


### Bug Fixes

* Updating WORKSPACE files to use the newest version of the Typescript generator. ([#172](https://www.github.com/googleapis/nodejs-ai-platform/issues/172)) ([d22eceb](https://www.github.com/googleapis/nodejs-ai-platform/commit/d22ecebc4693b63516a69753c19fa7a7a400464f))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
NimJay pushed a commit that referenced this pull request Nov 18, 2022
🤖 I have created a release \*beep\* \*boop\* 
---
## [2.1.0](https://www.github.com/googleapis/nodejs-datacatalog/compare/v2.0.0...v2.1.0) (2020-06-16)


### Features

* add order field to TagField and TagTemplateField ([#157](https://www.github.com/googleapis/nodejs-datacatalog/issues/157)) ([70c23c2](https://www.github.com/googleapis/nodejs-datacatalog/commit/70c23c2b609bd24901dc902090e23fbf92b55895))
* move ts target to es2018 from es2016 ([#172](https://www.github.com/googleapis/nodejs-datacatalog/issues/172)) ([e354dde](https://www.github.com/googleapis/nodejs-datacatalog/commit/e354ddeeeedd3e106c50ed579075f23c9d9c5bb5))
* promote library to GA ([#177](https://www.github.com/googleapis/nodejs-datacatalog/issues/177)) ([ac11090](https://www.github.com/googleapis/nodejs-datacatalog/commit/ac110906b723f362024318295e3de6743c905b7e))


### Bug Fixes

* proper fallback option handling ([51df672](https://www.github.com/googleapis/nodejs-datacatalog/commit/51df6721306ab6d2d9bf6919b0f3f432ca85b4ab))
* regenerate unit tests ([#163](https://www.github.com/googleapis/nodejs-datacatalog/issues/163)) ([4593117](https://www.github.com/googleapis/nodejs-datacatalog/commit/45931176d266d364e0508039cf6a4e284ad2923d))
* synth.py clean up for multiple version ([#167](https://www.github.com/googleapis/nodejs-datacatalog/issues/167)) ([c799604](https://www.github.com/googleapis/nodejs-datacatalog/commit/c799604b2d29269006ff39748afb78302ce46ca2))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
NimJay pushed a commit that referenced this pull request Nov 18, 2022
🤖 I have created a release \*beep\* \*boop\* 
---
## [2.1.0](https://www.github.com/googleapis/nodejs-datacatalog/compare/v2.0.0...v2.1.0) (2020-06-16)


### Features

* add order field to TagField and TagTemplateField ([#157](https://www.github.com/googleapis/nodejs-datacatalog/issues/157)) ([70c23c2](https://www.github.com/googleapis/nodejs-datacatalog/commit/70c23c2b609bd24901dc902090e23fbf92b55895))
* move ts target to es2018 from es2016 ([#172](https://www.github.com/googleapis/nodejs-datacatalog/issues/172)) ([e354dde](https://www.github.com/googleapis/nodejs-datacatalog/commit/e354ddeeeedd3e106c50ed579075f23c9d9c5bb5))
* promote library to GA ([#177](https://www.github.com/googleapis/nodejs-datacatalog/issues/177)) ([ac11090](https://www.github.com/googleapis/nodejs-datacatalog/commit/ac110906b723f362024318295e3de6743c905b7e))


### Bug Fixes

* proper fallback option handling ([51df672](https://www.github.com/googleapis/nodejs-datacatalog/commit/51df6721306ab6d2d9bf6919b0f3f432ca85b4ab))
* regenerate unit tests ([#163](https://www.github.com/googleapis/nodejs-datacatalog/issues/163)) ([4593117](https://www.github.com/googleapis/nodejs-datacatalog/commit/45931176d266d364e0508039cf6a4e284ad2923d))
* synth.py clean up for multiple version ([#167](https://www.github.com/googleapis/nodejs-datacatalog/issues/167)) ([c799604](https://www.github.com/googleapis/nodejs-datacatalog/commit/c799604b2d29269006ff39748afb78302ce46ca2))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please).
pattishin pushed a commit that referenced this pull request Nov 22, 2022
🤖 I have created a release \*beep\* \*boop\*
---
### [1.9.1](https://www.github.com/googleapis/nodejs-ai-platform/compare/v1.9.0...v1.9.1) (2021-07-21)


### Bug Fixes

* Updating WORKSPACE files to use the newest version of the Typescript generator. ([#172](https://www.github.com/googleapis/nodejs-ai-platform/issues/172)) ([d22eceb](https://www.github.com/googleapis/nodejs-ai-platform/commit/d22ecebc4693b63516a69753c19fa7a7a400464f))
---


This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please).
jsimonweb pushed a commit to jsimonweb/nodejs-docs-samples that referenced this pull request Dec 12, 2022
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