Skip to content

Conversation

RyanCavanaugh
Copy link
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


const targetCallSignature = checker.getResolvedSignature(node);
if (!targetCallSignature) {
if (!node.arguments) {
Copy link
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.

@@ -123,7 +123,7 @@ namespace ts {
try {
// 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

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
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?
🦀 👽

const current = currently ? ` (currently '${currently}')` : "";
return `Tag boolean argument as '${name}'${current}`;
}

public apply(sourceFile: ts.SourceFile): Lint.RuleFailure[] {
// Cheat to get type checker
Copy link
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