Skip to content

Update linting to avoid typechecking for booleanTriviaRule#15021

Merged
RyanCavanaugh merged 4 commits into
microsoft:masterfrom
RyanCavanaugh:lintFixes
Apr 5, 2017
Merged

Update linting to avoid typechecking for booleanTriviaRule#15021
RyanCavanaugh merged 4 commits into
microsoft:masterfrom
RyanCavanaugh:lintFixes

Conversation

@RyanCavanaugh
Copy link
Copy Markdown
Member

I fell down a bit of a rabbit hole! 🐇

Our boolean trivia rule was... interesting

  • It only could check intra-file calls; calls to other files were not checked for comments
  • It created a typechecker per file
  • It used a function that is not in the master version of TS Lint
  • It consumed approximately 65 of the 85 seconds it takes to lint the project

So given all these things, the new boolean trivia rule

  • Does not enforce that the parameter name matches the comment; the comment must simply exist (please be responsible)
  • No longer uses a typechecker (you can have your 65 seconds back in return)
  • Opts out certain methods and functions whose parameter names are not informative at call sites

As well I fixed all the lint errors that the next version of TS Lint will give us on release since I was npm link'd anyway

Comment thread scripts/tslint/booleanTriviaRule.ts Outdated

const targetCallSignature = checker.getResolvedSignature(node);
if (!targetCallSignature) {
if (!node.arguments) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Is it possible for CallExpression.arguments to ever be undefined?
As long as you don't handle NewExpression here you shouldn't need to worry about undefined.

// trigger synchronization to make sure that LSHost will try to find 'f2' module on disk
project.getLanguageService().getSemanticDiagnostics(imported.name);
assert.isTrue(false, `should not find file '${imported.name}'`);
assert.fail(false, `should not find file '${imported.name}'`);
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

assert.fail takes actual and expected as parameters. Doesn't seem like there's an overload that just takes a message. assert(false, msg) is probably best.

Copy link
Copy Markdown
Member

@sandersn sandersn left a comment

Choose a reason for hiding this comment

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

Typescript you say? Why not named arguments?
🦀 👽

Comment thread scripts/tslint/booleanTriviaRule.ts Outdated
}

public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
// Cheat to get type checker
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I don't think we're cheating OR getting a type checker any more.

@RyanCavanaugh RyanCavanaugh merged commit d8a24e3 into microsoft:master Apr 5, 2017
@RyanCavanaugh RyanCavanaugh deleted the lintFixes branch April 5, 2017 19:27
@microsoft microsoft locked and limited conversation to collaborators Jun 21, 2018
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.

4 participants