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

TypeScript: Turn on noImplicityAny and fix a few type signatures #587

Merged
merged 5 commits into from
Feb 2, 2020

Conversation

LucasHill
Copy link
Contributor

I was debugging some type issues I was having and decided to turn on 'noImplicityAny' (considered a best practice in TS). This should tighten up the types to be a lot more useful. I tried to make minimal actual code changes and just do types in this pass.

@LucasHill
Copy link
Contributor Author

@Turbo87 is there any appetite for this change?

@Turbo87
Copy link
Collaborator

Turbo87 commented Jan 2, 2020

generally, yes. but I haven't had time yet to review it properly 😞

@LucasHill
Copy link
Contributor Author

Ok thanks!

Copy link
Collaborator

@scalvert scalvert left a comment

Choose a reason for hiding this comment

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

These changes look largely good to me. I'm sure you feel the same way, but it's mildly annoying to have to @ts-ignore all those lines to explicitly opt out of type checking for those tests that are explicitly checking invalid parameter types. I don't have another solution for this, so we'll have to live with it.

lib/assertions.ts Outdated Show resolved Hide resolved
@scalvert
Copy link
Collaborator

@LucasHill thanks for doing this!

Copy link
Contributor Author

@LucasHill LucasHill left a comment

Choose a reason for hiding this comment

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

Agreed, all the ts-ignores were unfortunate but they are valid test cases for JS users, I don't know of a more elegant way to handle it :(

@LucasHill LucasHill force-pushed the use-no-implicit-any branch from 8475eac to 8f171bd Compare January 18, 2020 20:23
@LucasHill
Copy link
Contributor Author

@scalvert updated! thanks for the feedback

lib/assertions.ts Outdated Show resolved Hide resolved
lib/assertions.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@scalvert scalvert left a comment

Choose a reason for hiding this comment

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

Just a couple more things I noticed.

Copy link
Collaborator

@Turbo87 Turbo87 left a comment

Choose a reason for hiding this comment

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

thanks! I've adjusted a few minor things, but overall this looks 👍

@Turbo87 Turbo87 changed the title Turn on noImplicityAny and fix types TypeScript: Turn on noImplicityAny and fix a few type signatures Feb 2, 2020
@Turbo87 Turbo87 merged commit 36441fe into mainmatter:master Feb 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants