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

fix: add publicUrl to exclude promisifyAll #1339

Conversation

fernandolguevara
Copy link
Contributor

@fernandolguevara fernandolguevara commented Nov 16, 2020

Fixes #1344

I exclude "publicUrl" from the list of functions that will use promises

@fernandolguevara fernandolguevara requested a review from a team as a code owner November 16, 2020 17:33
@google-cla
Copy link

google-cla bot commented Nov 16, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla google-cla bot added the cla: no This human has *not* signed the Contributor License Agreement. label Nov 16, 2020
@product-auto-label product-auto-label bot added the api: storage Issues related to the googleapis/nodejs-storage API. label Nov 16, 2020
@AVaksman
Copy link
Contributor

@fernandolguevara Thank you for contribution.

Just a couple of nits.

src/file.ts Outdated
@@ -3798,7 +3798,7 @@ class File extends ServiceObject<File> {
* that a callback is omitted.
*/
promisifyAll(File, {
exclude: ['request', 'setEncryptionKey'],
exclude: ['request', 'setEncryptionKey', 'publicUrl'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Please place 'publicUrl' in alphabetic order.

Also please add 'publicUrl' to the list to fix unit tests

assert.deepStrictEqual(options.exclude, ['request', 'setEncryptionKey']);

@google-cla
Copy link

google-cla bot commented Nov 23, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@google-cla
Copy link

google-cla bot commented Nov 23, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@codecov
Copy link

codecov bot commented Nov 23, 2020

Codecov Report

Merging #1339 (676a268) into master (91a65f8) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1339   +/-   ##
=======================================
  Coverage   99.00%   99.00%           
=======================================
  Files          14       14           
  Lines       11804    11804           
  Branches      526      595   +69     
=======================================
  Hits        11687    11687           
  Misses        117      117           
Impacted Files Coverage Δ
src/file.ts 99.94% <100.00%> (ø)

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 91a65f8...676a268. Read the comment docs.

test/file.ts Outdated Show resolved Hide resolved
@google-cla
Copy link

google-cla bot commented Nov 30, 2020

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@shaffeeullah
Copy link
Contributor

@fernandolguevara friendly bump. If you do not have time to implement the requested changes, let me know and I can take it over from here. However, please sign the CLA so this PR can be merged. Thanks for making these changes!

@fernandolguevara
Copy link
Contributor Author

@googlebot I signed it!

@google-cla google-cla bot added cla: yes This human has signed the Contributor License Agreement. and removed cla: no This human has *not* signed the Contributor License Agreement. labels Dec 1, 2020
Co-authored-by: Sameena Shaffeeullah <[email protected]>
@stephenplusplus stephenplusplus added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 1, 2020
@yoshi-kokoro yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Dec 1, 2020
@stephenplusplus stephenplusplus merged commit ea2c2c9 into googleapis:master Dec 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: storage Issues related to the googleapis/nodejs-storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

file.publicUrl() returns a Promise instead of the publicUrl
5 participants