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

refactor(typescript): noImplicityAny for topic.ts and subscription.ts #420

Merged

Conversation

praveenqlogic
Copy link
Contributor

Fixes #<issue_number_goes_here> (it's a good idea to open an issue first for discussion)

steps
enable noImplicityAny, for subscription.ts and topic.ts file in SRC folder!

  • 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 Jan 11, 2019
Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

This is looking great! One piece missing though is a.) how you generate the pubsub.d.ts, and b.) making sure it gets copied to the output directory. Please see https://github.com/googleapis/nodejs-logging/blob/master/package.json for an example of how to do both :)

src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/topic.ts Outdated Show resolved Hide resolved
@praveenqlogic
Copy link
Contributor Author

@JustinBeckwith Hi, i generated a pubsub.d.ts using a below script

"mkdir -p proto && pbjs -t static-module -w commonjs -p node_modules/google-proto-files google/pubsub/v1/pubsub.proto | pbts -o proto/pubsub.d.ts -"

please let me know if i need to commit anything in this regard!

@AVaksman
Copy link
Contributor

@praveenqlogic please add "proto" and "proto:pubsub" to scripts section of package.json as in logging example here

src/subscription.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
src/subscription.ts Outdated Show resolved Hide resolved
src/subscription.ts Outdated Show resolved Hide resolved
src/subscription.ts Outdated Show resolved Hide resolved
src/topic.ts Outdated Show resolved Hide resolved
src/topic.ts Show resolved Hide resolved
@AVaksman
Copy link
Contributor

AVaksman commented Jan 14, 2019

src/subscription.ts Outdated Show resolved Hide resolved
src/subscription.ts Outdated Show resolved Hide resolved
src/subscription.ts Show resolved Hide resolved
src/subscription.ts Show resolved Hide resolved
src/subscription.ts Show resolved Hide resolved
src/topic.ts Outdated Show resolved Hide resolved
src/index.ts Outdated Show resolved Hide resolved
@praveenqlogic praveenqlogic force-pushed the nodejs-pubsub-tb-dev-01 branch from f62c061 to fcd6628 Compare January 15, 2019 21:10
@praveenqlogic
Copy link
Contributor Author

praveenqlogic commented Jan 18, 2019

@JustinBeckwith Hi, let me know if any changes needed on this PR, Hope you like the changes and approve it.

@JustinBeckwith JustinBeckwith changed the title noImplicityAny for topic.ts and subscription.ts file refactor(typescript): noImplicityAny for topic.ts and subscription.ts Jan 21, 2019
@josecervino
Copy link

(To create a reference to issue page) Part of #313

src/subscription.ts Outdated Show resolved Hide resolved
src/subscription.ts Outdated Show resolved Hide resolved
src/subscription.ts Outdated Show resolved Hide resolved
src/subscription.ts Outdated Show resolved Hide resolved
src/subscription.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@JustinBeckwith JustinBeckwith left a comment

Choose a reason for hiding this comment

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

We're so close!

src/topic.ts Show resolved Hide resolved
src/topic.ts Show resolved Hide resolved
@JustinBeckwith JustinBeckwith removed the cla: yes This human has signed the Contributor License Agreement. label Jan 25, 2019
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jan 25, 2019
@JustinBeckwith JustinBeckwith merged commit 4046864 into googleapis:master Jan 25, 2019
praveenqlogic added a commit to praveenqlogic/nodejs-pubsub that referenced this pull request Jan 30, 2019
feywind pushed a commit to feywind/nodejs-pubsub that referenced this pull request Nov 12, 2024
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.

6 participants