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(typescript): correct response type of Subscription.get #525

Merged
merged 2 commits into from
Mar 6, 2019

Conversation

merlinnot
Copy link
Contributor

Resolves #507

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Mar 5, 2019
@callmehiphop callmehiphop added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 5, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 5, 2019
@callmehiphop
Copy link
Contributor

Looks like we're getting a small lint error

src/topic.ts:181:49 - error TS2345: Argument of type 'ResourceCallback<Topic, ITopic> | undefined' is not assignable to parameter of type 'ResourceCallback<Topic, ITopic>'.
  Type 'undefined' is not assignable to type 'ResourceCallback<Topic, ITopic>'.

181     this.pubsub.createTopic(this.name, gaxOpts, callback);

I think we just need to make the following edit

this.pubsub.createTopic(this.name, gaxOpts, callback!);

@merlinnot
Copy link
Contributor Author

The call relied on an internal implementation (checking if the argument is equal to undefined), so I made it more correct so it actually conforms to a function interface.

@callmehiphop callmehiphop added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2019
@callmehiphop
Copy link
Contributor

@merlinnot we actually use a decorator to promisify everything, so callback will never be undefined, however we've been marking it as optional for the Promise signature overload.

@codecov
Copy link

codecov bot commented Mar 6, 2019

Codecov Report

Merging #525 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #525   +/-   ##
=======================================
  Coverage   94.95%   94.95%           
=======================================
  Files          16       16           
  Lines         992      992           
  Branches       87       87           
=======================================
  Hits          942      942           
  Misses         42       42           
  Partials        8        8
Impacted Files Coverage Δ
src/subscription.ts 100% <ø> (ø) ⬆️
src/pubsub.ts 100% <ø> (ø) ⬆️
src/topic.ts 91.25% <100%> (ø) ⬆️

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 542ac65...24fe9f9. Read the comment docs.

@merlinnot
Copy link
Contributor Author

Ok, changed and rebased on master.

@callmehiphop callmehiphop added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2019
@callmehiphop
Copy link
Contributor

@merlinnot awesome, thank you so much!

@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2019
@merlinnot
Copy link
Contributor Author

I can't see CI logs, both lint and compile work for me locally. It's hard to contribute when only Googlers see build logs ;)

@callmehiphop
Copy link
Contributor

@merlinnot yeah T_T I don't think there's anything wrong with your code, Kokoro (CI) has been super flakey today. @JustinBeckwith do we have any idea what's going on with it?

@JustinBeckwith JustinBeckwith added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2019
@callmehiphop callmehiphop added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2019
@callmehiphop callmehiphop added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2019
@kokoro-team kokoro-team removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Mar 6, 2019
@callmehiphop callmehiphop merged commit 416ef66 into googleapis:master Mar 6, 2019
@merlinnot merlinnot deleted the fix-issue-507-part-1 branch March 7, 2019 00:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants