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

re-enable gts check and linting #311

Closed
JustinBeckwith opened this issue Nov 2, 2018 · 4 comments · Fixed by #375
Closed

re-enable gts check and linting #311

JustinBeckwith opened this issue Nov 2, 2018 · 4 comments · Fixed by #375
Assignees
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. help wanted We'd love to have community involvement on this issue. type: cleanup An internal cleanup or hygiene concern. TypeScript

Comments

@JustinBeckwith
Copy link
Contributor

JustinBeckwith commented Nov 2, 2018

The work here is to get npx gts fix passing. Then the lint command should do a gts check, and the fix command should do a gts fix. This will be a very large change, so it's ok to do it in small chunks.

@JustinBeckwith JustinBeckwith added type: cleanup An internal cleanup or hygiene concern. TypeScript labels Nov 2, 2018
@JustinBeckwith JustinBeckwith added the help wanted We'd love to have community involvement on this issue. label Nov 16, 2018
@vijay-qlogic
Copy link
Contributor

@JustinBeckwith type "any" is in use at multiple places, and as per "/tslint.json" which extends "gts/tslint.json", follows "no-any" rule.

At some places it's hard to decide if type "any" should be removed,
like: src/subscriber.ts ln 129 col 67

Shall I set rule { "no-any": false } in "tslint.json"?

@JustinBeckwith
Copy link
Contributor Author

Heh, nah. This is going to hurt. What you do is set noImplicitAny to true in the tsconfig. That's going to break a lot of stuff. You start with one file, and add types to fix pretty much all of the ambiguous types. Then, you turn noImplicitAny back off, and submit a PR for just that file.

Here are a few examples of repos that have been transitioned in this way:

It's admittedly a tad tedious. Generally the only places you should use any are in tests.

@vijay-qlogic
Copy link
Contributor

@JustinBeckwith Thanks!
But raising PR for individual file-change my break the stuff, for changes like:
export class IAM { Promise?: PromiseConstructor;
to
export class IAM { promise?: PromiseConstructor;
that is changing the property name.

Any concerns with raising PR for all changes together?

@JustinBeckwith
Copy link
Contributor Author

So we absolutely do not want to introduce breaking changes here. In this example, we would us a tslint ignore. Place this on the pine above the property.

// tslint:disable-next-line variable-name

If we don't limit this to one file at a time, the PR will quickly get out of control. I'd the changes to other files are small and required to get basic compilation going that's fine.

@vijay-qlogic vijay-qlogic self-assigned this Nov 26, 2018
@google-cloud-label-sync google-cloud-label-sync bot added the api: pubsub Issues related to the googleapis/nodejs-pubsub API. label Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api: pubsub Issues related to the googleapis/nodejs-pubsub API. help wanted We'd love to have community involvement on this issue. type: cleanup An internal cleanup or hygiene concern. TypeScript
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants