Skip to content
This repository has been archived by the owner on Mar 25, 2021. It is now read-only.

Add unknown as simple type to array-type #4433

Merged
merged 1 commit into from Jan 3, 2019

Conversation

ThomasdenH
Copy link
Contributor

@ThomasdenH ThomasdenH commented Jan 3, 2019

PR checklist

Overview of change:

Add unknown to the simple types. Updated tests to test this functionality.

CHANGELOG.md entry:

[bugfix] unknown is recognized as simple type in array-type

@palantirtech
Copy link
Member

Thanks for your interest in palantir/tslint, @ThomasdenH! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

@ThomasdenH ThomasdenH changed the title [WIP] Add unknown as simple type to array-type Add unknown as simple type to array-type Jan 3, 2019
@ThomasdenH
Copy link
Contributor Author

I had marked this as WIP because I expected the unknown keyword to be undefined on older versions. But it seems to work in CI.

Copy link
Contributor

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

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

Awesome, thanks @ThomasdenH! Glad some kind of horrible hack around TypeScript versions wasn't necessary after all. 😄

What's probably happening: builds running with older versions of TypeScript would have type checking errors for an unknown name type in TS if we asked for them. TSLint still runs even if there are syntax or type errors so these are ignored. ts.SyntaxKind.UnknownKeyword will resolve to the right value if a new version of TS is resolved at runtime or undefined if an older version is.

@JoshuaKGoldberg JoshuaKGoldberg merged commit 536b80f into palantir:master Jan 3, 2019
@ThomasdenH
Copy link
Contributor Author

I see. Well, it should make the other unknown issues simple to do as well!

@ThomasdenH
Copy link
Contributor Author

Is it possible you missed this in the changelog?

@adidahiya
Copy link
Contributor

ah, I actually missed it when cherry-picking commits onto the release branch. I'll get it in the next patch

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants