I think we should require a callback, otherwise there's not a point to calling the method. One method that doesn't require a callback, like "restore()", the user could ignore the response, but the actions caused by the API request would still persist.

Making callback require user will not able to use await here What do you think?

Could you explain that further? If it relates, all of our methods require a callback that promisifyAll uses to convert the callback-written function into a promise-based function, that is then compatible with an async workflow.

Yes we are using promisifyAll to convert promise-based function.
I have seen in all repo we are doing same pattern, let me know if you have working example/reference of async workflow will make change accordingly.
Thanks!!

The only thing I'm pointing out in this case, is that the callback should not be optional. Here's a comparable function: nodejs-compute/vm#attachDisk() https://github.com/googleapis/nodejs-compute/blob/2484e4b6521fdc9dce0c80b6e8bb4f3da7baf7df/src/vm.js#316

It accepts an optional options argument, and requires a callback.

All of these are compatible with a user who doesn't actually provide a function callback, instead using the promise/async flow.

It seems that nodejs-compute is still js and, once converted and annotate with typescript types, the callback will have to be marked as optional as well. From user's perspective, callback is optional. It will nevertheless always be there where as from user or from primisify.

This existing test it('should not require a callback', () => {...}); doesn't really test the functionality of this library, but rathe functionality of promisify and might not be needed here at all. WDYT?

This is definitely hard to follow between all of our layers 👨💥

The test is making sure that "callback = callback || util.noop" is there, which is what I think should not be there. I don't know if it makes a difference after promisify overwrites the method?, but if not, we could just assume there is always a callback, and remove the "defaulting to noop" line, and remove the test?

The user seeing it in the docs or annotations as optional is good, but the code falling back to a noop seems misleading.

removed callback = callback || util.noop statement and test that was checking the callback.