Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

refactor(ts): enable noImplicitAny on cluster.ts #452

Merged
merged 2 commits into from
Apr 12, 2019
Merged

refactor(ts): enable noImplicitAny on cluster.ts #452

merged 2 commits into from
Apr 12, 2019

Conversation

josecervino
Copy link

@josecervino josecervino commented Apr 4, 2019

Fixes #427 (4/21ish)

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

🤞

OPR 1 (4/4 @ 6:30pm): Initial draft submit

  • Codecov/patch drops to 89.65%
  • Codecov/project drops to 95.38% from 95.57%

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 4, 2019
export interface GenericCallback<T> {
(err?: ServiceError|null, apiResponse?: T|null): void;
}
export interface GenericClusterCallback<T> {
Copy link
Author

Choose a reason for hiding this comment

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

Not sure if this is necessary - I'm thinking that if these callbacks are only used once then a Generic isn't needed. Same in the cases where there are multiple callbacks with the same apiResponse. Let me know what you think!

@codecov
Copy link

codecov bot commented Apr 4, 2019

Codecov Report

Merging #452 into master will decrease coverage by 0.18%.
The diff coverage is 86.95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #452      +/-   ##
==========================================
- Coverage   95.57%   95.38%   -0.19%     
==========================================
  Files          14       14              
  Lines        1468     1473       +5     
  Branches       84      102      +18     
==========================================
+ Hits         1403     1405       +2     
  Misses         61       61              
- Partials        4        7       +3
Impacted Files Coverage Δ
src/cluster.ts 94% <86.95%> (-6%) ⬇️
src/chunktransformer.ts 100% <0%> (ø) ⬆️

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 e29f9aa...c2c77c3. Read the comment docs.

static getStorageType_(type) {
const storageTypes = {
static getStorageType_(type: string): number {
const storageTypes: {[k: string]: number} = {
Copy link
Author

Choose a reason for hiding this comment

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

Should I make a general-use interface for {[k: string]: number}?

const callback =
typeof gaxOptionsOrCallback === 'function' ? gaxOptionsOrCallback : cb!;
const gaxOptions =
typeof gaxOptionsOrCallback === 'object' && gaxOptionsOrCallback ?
Copy link
Author

Choose a reason for hiding this comment

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

I add && gaxOptionsOrCallback to filter for null - I'm thinking it's nice to have but I'd understand it not being necessary


this.getMetadata(gaxOptions, err => {
this.getMetadata(gaxOptions, (err?: ServiceError|null) => {
Copy link
Author

Choose a reason for hiding this comment

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

.getMetadata() callback param types not inferred :/

callback(err, err ? null : this, metadata);
});
this.getMetadata(
gaxOptions, (err?: ServiceError|null, metadata?: ICluster|null) => {
Copy link
Author

Choose a reason for hiding this comment

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

I'm thinking of ICluster and other respective I<class name> interfaces as metadata - is this the correct way to think about it?

@josecervino josecervino marked this pull request as ready for review April 10, 2019 19:19
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Apr 11, 2019
@JustinBeckwith JustinBeckwith merged commit 8682184 into googleapis:master Apr 12, 2019
@josecervino
Copy link
Author

Did my submission just get merged without change requests??

I want to thank my mom, my dog, Obama, and @callmehiphop for this honor 😭

@yoshi-automation yoshi-automation removed the 🚨 This issue needs some love. label Apr 7, 2020
# for free to join this conversation on GitHub. Already have an account? # 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.

Convert library to TypeScript
4 participants