refactor: remove usage of async module #702
Conversation
googlebot
added
the
cla: yes
Codecov Report
@@ Coverage Diff @@
## master #702 +/- ##
=======================================
Coverage 96.77% 96.77%
=======================================
Files 9 9
Lines 1149 1149
Branches 294 294
=======================================
Hits 1112 1112
Misses 9 9
Partials 28 28
Continue to review full report at Codecov.
|
AVaksman
force-pushed
the
remove_async_module
branch
from
a6db7e0
to
b33b94e
Compare
system-test/storage.ts
Outdated
| import * as crypto from 'crypto'; | ||
| import * as fs from 'fs'; | ||
| import fetch from 'node-fetch'; | ||
| const normalizeNewline = require('normalize-newline'); | ||
| const pLimit = require('p-limit'); |
There was a problem hiding this comment.
| const pLimit = require('p-limit'); | |
| import pLimit = require('p-limit'); |
system-test/storage.ts
Outdated
| import * as crypto from 'crypto'; | ||
| import * as fs from 'fs'; | ||
| import fetch from 'node-fetch'; | ||
| const normalizeNewline = require('normalize-newline'); | ||
| const pLimit = require('p-limit'); | ||
| const {promisify} = require('util'); |
There was a problem hiding this comment.
| const {promisify} = require('util'); | |
| import {promisify} from 'util'; |
There was a problem hiding this comment.
Fixed as well, thanks
system-test/storage.ts
Outdated
| }), | ||
| done | ||
| after(async () => { | ||
| Promise.all( |
There was a problem hiding this comment.
added await, thanks
system-test/storage.ts
Outdated
| age: 30, | ||
| isLive: true, | ||
| }, | ||
| }), |
There was a problem hiding this comment.
weird formatting - would expect a semicolon here
system-test/storage.ts
Outdated
| } | ||
| ); | ||
| await bucket.lock(bucket.metadata.metageneration); | ||
| try { |
There was a problem hiding this comment.
Please use an assert.rejects here so we're 100% sure the exception was thrown
system-test/storage.ts
Outdated
| ); | ||
| return Promise.all( | ||
| FILES.map(file => | ||
| file.setMetadata({temporaryHold: null}).then(() => file.delete()) |
There was a problem hiding this comment.
The mix of promises and async/await is weird. Can we just use an async function here and awaits?
There was a problem hiding this comment.
Changed that, thanks
system-test/storage.ts
Outdated
| }); | ||
| const [file] = await bucket.upload(FILES.logo.path, opts); | ||
| const [copiedFile] = await file!.copy('CloudLogoCopy'); | ||
| await Promise.all([file!.delete, copiedFile!.delete()]); |
There was a problem hiding this comment.
Should this be file.delete()?
There was a problem hiding this comment.
Yes it should, fixed, thanks
Thank you. |
|
@JustinBeckwith PTAL |
There was a problem hiding this comment.
This looks reasonable to me, if @JustinBeckwith has already given his 👍 I say ship it.
Towards googleapis/google-cloud-node/issues/2883