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

Make client args credentials and options optional #512

Merged
merged 3 commits into from
Mar 26, 2019

Conversation

jonahbron
Copy link
Collaborator

Currently the TypeScript generated clients require that null be passed to credentials and options if they're not being used. It's easier to simply make those arguments optional. I've left the null portion of the type though to maintain backwards compatibility.

Currently the TypeScript generated clients require that `null` be passed to `credentials` and `options` if they're not being used.  It's easier to simply make those arguments optional.  I've left the `null` portion of the type though to maintain backwards compatibility.
@jonahbron
Copy link
Collaborator Author

Not sure why Kokoro still hasn't completed its checks... but otherwise this PR is ready for review.

@stanley-cheung
Copy link
Collaborator

Yea I have to apply the tag for the kokoro test to run, and the reason I haven't applied the tag is because I thought we did this and reverted a long while ago and I forgot the reason now and I haven't got a chance to dig out the history. I might remember wrong. Will get to this asap.

@jonahbron
Copy link
Collaborator Author

Gotcha. Thanks @stanley-cheung , no rush.

@jonahbron
Copy link
Collaborator Author

@stanley-cheung I looked through the history of the master branch and found no evidence that those args were ever optional, outside of having been looser types in the past (i.e. any).

@stanley-cheung
Copy link
Collaborator

stanley-cheung commented Mar 26, 2019

After a bit of quick digging, what I had been thinking was #392 and #396. Looks like they are a different issue than this. Just want to be careful on not trying to do this A, revert A, do A again and repeat :)

Will review this again soon.

Copy link
Collaborator

@stanley-cheung stanley-cheung left a comment

Choose a reason for hiding this comment

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

LGTM. This is only for the typescript and commonjs+dts output. Thanks for the contributions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants