feat: allow options to ServiceObject methods #349
Conversation
stephenplusplus
added
the
do not merge
googlebot
added
the
cla: yes
src/service-object.ts
Outdated
| @@ -42,8 +42,13 @@ export interface Interceptor { | |||
| request(opts: r.Options): DecorateRequestOptions; | |||
| } | |||
|
|
|||
| // tslint:disable-next-line:no-any | |||
| export type GetMetadataOptions = any; | |||
There was a problem hiding this comment.
I think maybe it would be better to use object than any here. I think it is safe to assume that we'll only ever allow objects to be passed in (as opposed to strings, numbers, etc.).
src/service-object.ts
Outdated
| // tslint:disable-next-line:no-any | ||
| export type Metadata = any; | ||
| // tslint:disable-next-line:no-any | ||
| export type SetMetadataOptions = any; |
There was a problem hiding this comment.
See comments for GetMetadataOptions.
src/service-object.ts
Outdated
| optionsOrCallback?: GetMetadataOptions|MetadataCallback, | ||
| callback?: MetadataCallback): Promise<MetadataResponse>|void { | ||
| let options: GetMetadataOptions = {}; | ||
| if (typeof optionsOrCallback === 'function') { |
There was a problem hiding this comment.
Consider using maybeOptionsOrCallback to simplify this logic!
src/service-object.ts
Outdated
| metadata: Metadata, | ||
| optionsOrCallback?: SetMetadataOptions|MetadataCallback, | ||
| callback?: MetadataCallback): Promise<SetMetadataResponse>|void { | ||
| const options = |
There was a problem hiding this comment.
Consider using maybeOptionsOrCallback to simplify this logic!
src/service-object.ts
Outdated
| @@ -42,8 +42,13 @@ export interface Interceptor { | |||
| request(opts: r.Options): DecorateRequestOptions; | |||
| } | |||
|
|
|||
| // tslint:disable-next-line:no-any | |||
There was a problem hiding this comment.
I think you can get rid of this now?
| @@ -122,7 +123,7 @@ export class Operation<T = any> extends ServiceObject<T> { | |||
| * @private | |||
| */ | |||
| protected poll_(callback: MetadataCallback): void { | |||
| this.getMetadata((err, body) => { | |||
| this.getMetadata((err: ApiError, body: Metadata) => { | |||
There was a problem hiding this comment.
huh, the compiler should be able to infer the types for the callback. Weird that you had to add this.
stephenplusplus
removed
the
do not merge
|
@JustinBeckwith @callmehiphop -- tests added. PTAfinalL. It would be great to get this in before the next release (#350). |
To Dos
This allows
optionsto be passed to several methods on ServiceObject:For Storage, in order to support the
userProjectobject, we ended up duplicating many of these methods, so that we could pass extra parameters along with the request as query string parameters. With this change, we will just support that here, where it probably belongs.Sending this in early for feedback before I carry on with the other methods and tests.