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

Add typeof-compare rule #1927

Merged
merged 8 commits into from Jan 1, 2017
Merged

Conversation

mohsen1
Copy link
Contributor

@mohsen1 mohsen1 commented Dec 24, 2016

PR checklist

  • New feature, bugfix, or enhancement
    • Includes tests
  • Documentation update

What changes did you make?

Added typeof-compare rule. It makes sure result of typeof expressions are always compared to legal strings. I originally asked TypeScript to make comparing result of typeof to illegal strings a compiler error but per spec typeof can return anything!

Is there anything you'd like reviewers to focus on?

This is my first rule. I'm almost certain I'm missing stuff in this PR.

@mohsen1
Copy link
Contributor Author

mohsen1 commented Dec 29, 2016

How should I update documentation for this?

@nchen63
Copy link
Contributor

nchen63 commented Jan 1, 2017

you can run the doc generator by running npm run verify

@mohsen1
Copy link
Contributor Author

mohsen1 commented Jan 1, 2017

@natgabb done

~~~~~~~~~~~~~~~~~~~~~~~~~~ [typeof must be compared to correct value]

const string = "string";
typeof bar === string
Copy link
Contributor

Choose a reason for hiding this comment

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

needs more positive tests. Currently only tests against a string variable. Needs to test against directly against a string, against a template string, and a template string with a replacement variable. Also exercise different types of valid types

@mohsen1
Copy link
Contributor Author

mohsen1 commented Jan 1, 2017

Hmmm failures don't make sense to me

}

private static isLegalStringLiteral(node: ts.Node) {
return ComparisonWalker.LEGAL_TYPEOF_RESULTS.indexOf(node.getText()) > -1;
Copy link
Contributor

Choose a reason for hiding this comment

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

it's failing here because node.getText() includes the quotes. For example, "string" instead of what you want: string. If you use node.text, it will exclude the quotes

@nchen63 nchen63 merged commit 88d6083 into palantir:master Jan 1, 2017
@nchen63
Copy link
Contributor

nchen63 commented Jan 1, 2017

@mohsen1 thanks for the contribution!

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

2 participants