Skip to content

Fix as many linter errors as possible #4008

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.

Sign up for GitHub

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

Merged
DanielRosenwasser merged 3 commits into from
Aug 6, 2015
Merged

Fix as many linter errors as possible #4008

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5a6d476
Fix as many linter errors as possible
weswigham Jul 23, 2015
91f0346
Conform lint of the future, unify quotations
weswigham Jul 24, 2015
5324f8b
Fix new linter errors
weswigham Aug 4, 2015
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions src/compiler/binder.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,7 @@ namespace ts {
function getDeclarationName(node: Declaration): string {
if (node.name) {
if (node.kind === SyntaxKind.ModuleDeclaration && node.name.kind === SyntaxKind.StringLiteral) {
return '"' + (<LiteralExpression>node.name).text + '"';
return `"${(<LiteralExpression>node.name).text}"`;
}
if (node.name.kind === SyntaxKind.ComputedPropertyName) {
let nameExpression = (<ComputedPropertyName>node.name).expression;
Expand Down Expand Up @@ -830,7 +830,7 @@ namespace ts {

// Note: the node text must be exactly "use strict" or 'use strict'. It is not ok for the
// string to contain unicode escapes (as per ES5).
return nodeText === '"use strict"' || nodeText === "'use strict'";
return nodeText === "\"use strict\"" || nodeText === "'use strict'";

@danquirk danquirk Jul 24, 2015

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.

this seems like the kind of place you want to mix quotes instead of using escapes (except for the hilarious inversion of ' and " on each side of the ||)

@weswigham weswigham Jul 24, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

There's a number of places where that's true... but quote consistency is there for quote consistency, not for convenience, I suppose.

@danquirk danquirk Jul 24, 2015

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.

Yeah I saw a bunch more and didn't bother commenting again. I don't feel super strongly here but I always found mixing ' and " handy for cases like this but I can live without it.

@weswigham weswigham Jul 24, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I've actually been wondering this for awhile - since template strings are a thing in JS now, why don't linters have an option to prefer those? I think they're a bit more flexible and generally easier to keep consistent with. Plus, it reduces what you have as far as string literals go from quotes and backticks to just backticks. (Since you're likely using backticks for interpolation anyway!)

@ashwinr ashwinr Jul 24, 2015

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@weswigham TSLint supports string templates for the quote rule -- it does this by simply ignoring the contents of template strings. So a "double" quote rule really means only allow double quotes or string templates.

@weswigham weswigham Jul 24, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, I mean, why not forbid all quotes (single and double)? Template string literals can function as string literals handily, and it further unifies what string literals get used. Not really relevant here, I was just wondering why no linting tools seemed to support this option.

@adidahiya adidahiya Aug 1, 2015

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.

@weswigham that's a valid suggestion. filed that feature request here: palantir/tslint#539

@DanielRosenwasser DanielRosenwasser Aug 5, 2015

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'd still prefer to be able to mix quotes. I think it would be preferable for TSLint to support "double quotes unless the string contains a double quote."

@weswigham weswigham Aug 5, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Wouldn't be a problem if you used backticks for all the quotes. Until you needed to include backticks. 😄

But no really - try not to mix quotes. It makes sense not to mix quotes if you consider searching for strings in the source code. Knowing the exact quotes used lets you (plaintext) search with much higher precision - knowing you can always Ctrl-Shift-F "Joe's "Error" Message" (over 'Joe's "Error" Message') is way easier than not knowing if it's in one of many possible string formats. Generally speaking it's worth the effort of needing to toss in a \ in your string literals every so often.

}

function bindWorker(node: Node) {
Expand Down Expand Up @@ -930,7 +930,7 @@ namespace ts {
function bindSourceFileIfExternalModule() {
setExportContextFlag(file);
if (isExternalModule(file)) {
bindAnonymousDeclaration(file, SymbolFlags.ValueModule, '"' + removeFileExtension(file.fileName) + '"');
bindAnonymousDeclaration(file, SymbolFlags.ValueModule, `"${removeFileExtension(file.fileName)}"`);
}
}

Expand Down
15 changes: 8 additions & 7 deletions src/compiler/checker.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,9 @@ namespace ts {
let assignableRelation: Map<RelationComparisonResult> = {};
let identityRelation: Map<RelationComparisonResult> = {};

// This is for caching the result of getSymbolDisplayBuilder. Do not access directly.
let _displayBuilder: SymbolDisplayBuilder;

type TypeSystemEntity = Symbol | Type | Signature;

const enum TypeSystemPropertyName {
Expand Down Expand Up @@ -965,7 +968,7 @@ namespace ts {
if (!moduleName) return;
let isRelative = isExternalModuleNameRelative(moduleName);
if (!isRelative) {
let symbol = getSymbol(globals, '"' + moduleName + '"', SymbolFlags.ValueModule);
let symbol = getSymbol(globals, "\"" + moduleName + "\"", SymbolFlags.ValueModule);
if (symbol) {
return symbol;
}
Expand Down Expand Up @@ -1490,8 +1493,6 @@ namespace ts {
return undefined;
}

@DanielRosenwasser DanielRosenwasser Jul 23, 2015

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.

This was reported by a linter error? I think the original location was more appropriate.

@weswigham weswigham Jul 23, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

It was. It was unreachable code - if this was ever compiled to a let in the result rather than a var, the resulting TS wouldn't have run correctly. :D

let declarations after a return statement are unreachable and are therefore never scoped, unlike a var which would be hoisted with the function definition.

So you can say the linter found a bug waiting to happen here.

@DanielRosenwasser DanielRosenwasser Jul 23, 2015

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.

Ah I see; nice!. Perhaps it would be better to just make it a var declaration again.

@weswigham weswigham Jul 23, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

If we do that it'll have to be wrapped:

/* tslint:disable:no-var-keyword */
// This is for caching the result of getSymbolDisplayBuilder. Do not access directly.
var _displayBuilder: SymbolDisplayBuilder;
/* tslint:enable:no-var-keyword */

Is it worth it?

@danquirk danquirk Jul 23, 2015

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.

No one is going to want that amount of verbosity to disable things, especially one liners

@weswigham weswigham Jul 23, 2015

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I thought that was the point? You're not supposed to disable the linter.

@danquirk danquirk Jul 23, 2015

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.

Yeah there is an interesting push and pull in this sort of thing. You can make the syntax to disable things as gross as possible to disincentivize disabling, on the other hand you might accept that disabling things is a fact of life and it shouldn't look like an atrocity.

This is a good example of what I was mentioning to @DanielRosenwasser as far as how I only fixed the easy 80-90% cases as the rest that require non-trivial rewrites will generate some amount of discussion on whether we feel some particular situation warrants an exception.

My opinion is just make the change to satisfy the linter. While I see the argument for the 'define as close to usage as possible' it's not hard to use GoToDef occasionally vs adding gross comments.


// This is for caching the result of getSymbolDisplayBuilder. Do not access directly.
let _displayBuilder: SymbolDisplayBuilder;
function getSymbolDisplayBuilder(): SymbolDisplayBuilder {

function getNameOfSymbol(symbol: Symbol): string {
Expand Down Expand Up @@ -3448,7 +3449,7 @@ namespace ts {
// type, a property is considered known if it is known in any constituent type.
function isKnownProperty(type: Type, name: string): boolean {
if (type.flags & TypeFlags.ObjectType && type !== globalObjectType) {
var resolved = resolveStructuredTypeMembers(type);
const resolved = resolveStructuredTypeMembers(type);
return !!(resolved.properties.length === 0 ||
resolved.stringIndexType ||
resolved.numberIndexType ||
Expand Down Expand Up @@ -7337,7 +7338,7 @@ namespace ts {
}

// Wasn't found
error(node, Diagnostics.Property_0_does_not_exist_on_type_1, (<Identifier>node.tagName).text, 'JSX.' + JsxNames.IntrinsicElements);
error(node, Diagnostics.Property_0_does_not_exist_on_type_1, (<Identifier>node.tagName).text, "JSX." + JsxNames.IntrinsicElements);
return unknownSymbol;
}
else {
Expand Down Expand Up @@ -7377,7 +7378,7 @@ namespace ts {
function getJsxElementInstanceType(node: JsxOpeningLikeElement) {
// There is no such thing as an instance type for a non-class element. This
// line shouldn't be hit.
Debug.assert(!!(getNodeLinks(node).jsxFlags & JsxFlags.ClassElement), 'Should not call getJsxElementInstanceType on non-class Element');
Debug.assert(!!(getNodeLinks(node).jsxFlags & JsxFlags.ClassElement), "Should not call getJsxElementInstanceType on non-class Element");

let classSymbol = getJsxElementTagSymbol(node);
if (classSymbol === unknownSymbol) {
Expand Down Expand Up @@ -7557,7 +7558,7 @@ namespace ts {
// be marked as 'used' so we don't incorrectly elide its import. And if there
// is no 'React' symbol in scope, we should issue an error.
if (compilerOptions.jsx === JsxEmit.React) {
let reactSym = resolveName(node.tagName, 'React', SymbolFlags.Value, Diagnostics.Cannot_find_name_0, 'React');
let reactSym = resolveName(node.tagName, "React", SymbolFlags.Value, Diagnostics.Cannot_find_name_0, "React");
if (reactSym) {
getSymbolLinks(reactSym).referenced = true;
}
Expand Down
4 changes: 2 additions & 2 deletions src/compiler/commandLineParser.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -335,7 +335,7 @@ namespace ts {
* @param fileName The path to the config file
*/
export function readConfigFile(fileName: string): { config?: any; error?: Diagnostic } {
let text = '';
let text = "";
try {
text = sys.readFile(fileName);
}
Expand Down Expand Up @@ -423,7 +423,7 @@ namespace ts {
fileNames = map(<string[]>json["files"], s => combinePaths(basePath, s));
}
else {
errors.push(createCompilerDiagnostic(Diagnostics.Compiler_option_0_requires_a_value_of_type_1, 'files', 'Array'));
errors.push(createCompilerDiagnostic(Diagnostics.Compiler_option_0_requires_a_value_of_type_1, "files", "Array"));
}
}
else {
Expand Down
14 changes: 7 additions & 7 deletions src/compiler/core.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,10 +219,10 @@ namespace ts {
export function reduceLeft<T, U>(array: T[], f: (a: U, x: T) => U, initial: U): U;
export function reduceLeft<T, U>(array: T[], f: (a: U, x: T) => U, initial?: U): U {
if (array) {
var count = array.length;
const count = array.length;
if (count > 0) {
var pos = 0;
var result = arguments.length <= 2 ? array[pos++] : initial;
let pos = 0;
let result = arguments.length <= 2 ? array[pos++] : initial;
while (pos < count) {
result = f(<U>result, array[pos++]);
}
Expand All @@ -236,9 +236,9 @@ namespace ts {
export function reduceRight<T, U>(array: T[], f: (a: U, x: T) => U, initial: U): U;
export function reduceRight<T, U>(array: T[], f: (a: U, x: T) => U, initial?: U): U {
if (array) {
var pos = array.length - 1;
let pos = array.length - 1;
if (pos >= 0) {
var result = arguments.length <= 2 ? array[pos--] : initial;
let result = arguments.length <= 2 ? array[pos--] : initial;
while (pos >= 0) {
result = f(<U>result, array[pos--]);
}
Expand Down Expand Up @@ -523,7 +523,7 @@ namespace ts {
if (path.lastIndexOf("file:///", 0) === 0) {
return "file:///".length;
}
let idx = path.indexOf('://');
let idx = path.indexOf("://");
if (idx !== -1) {
return idx + "://".length;
}
Expand Down Expand Up @@ -805,4 +805,4 @@ namespace ts {
Debug.assert(false, message);
}
}
}
}
61 changes: 31 additions & 30 deletions src/compiler/emitter.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,7 +380,7 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
function base64VLQFormatEncode(inValue: number) {
function base64FormatEncode(inValue: number) {
if (inValue < 64) {
return 'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/'.charAt(inValue);
return "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789+/".charAt(inValue);
}
throw TypeError(inValue + ": not a 64 based value");
}
Expand Down Expand Up @@ -895,7 +895,7 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
// Any template literal or string literal with an extended escape
// (e.g. "\u{0067}") will need to be downleveled as a escaped string literal.
if (languageVersion < ScriptTarget.ES6 && (isTemplateLiteralKind(node.kind) || node.hasExtendedUnicodeEscape)) {
return getQuotedEscapedLiteralText('"', node.text, '"');
return getQuotedEscapedLiteralText("\"", node.text, "\"");
}

// If we don't need to downlevel and we can reach the original source text using
Expand All @@ -908,15 +908,15 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
// or an escaped quoted form of the original text if it's string-like.
switch (node.kind) {
case SyntaxKind.StringLiteral:
return getQuotedEscapedLiteralText('"', node.text, '"');
return getQuotedEscapedLiteralText("\"", node.text, "\"");
case SyntaxKind.NoSubstitutionTemplateLiteral:
return getQuotedEscapedLiteralText('`', node.text, '`');
return getQuotedEscapedLiteralText("`", node.text, "`");
case SyntaxKind.TemplateHead:
return getQuotedEscapedLiteralText('`', node.text, '${');
return getQuotedEscapedLiteralText("`", node.text, "${");
case SyntaxKind.TemplateMiddle:
return getQuotedEscapedLiteralText('}', node.text, '${');
return getQuotedEscapedLiteralText("}", node.text, "${");
case SyntaxKind.TemplateTail:
return getQuotedEscapedLiteralText('}', node.text, '`');
return getQuotedEscapedLiteralText("}", node.text, "`");
case SyntaxKind.NumericLiteral:
return node.text;
}
Expand Down Expand Up @@ -947,7 +947,7 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
text = text.replace(/\r\n?/g, "\n");
text = escapeString(text);

write('"' + text + '"');
write(`"${text}"`);
}

function emitDownlevelTaggedTemplateArray(node: TaggedTemplateExpression, literalEmitter: (literal: LiteralExpression) => void) {
Expand Down Expand Up @@ -1134,9 +1134,9 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
/// 'Div' for upper-cased or dotted names
function emitTagName(name: Identifier|QualifiedName) {
if (name.kind === SyntaxKind.Identifier && isIntrinsicJsxName((<Identifier>name).text)) {
write('"');
write("\"");
emit(name);
write('"');
write("\"");
}
else {
emit(name);
Expand All @@ -1148,9 +1148,9 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
/// about keywords, just non-identifier characters
function emitAttributeName(name: Identifier) {
if (/[A-Za-z_]+[\w*]/.test(name.text)) {
write('"');
write("\"");
emit(name);
write('"');
write("\"");
}
else {
emit(name);
Expand Down Expand Up @@ -1248,10 +1248,10 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
// Don't emit empty strings
if (children[i].kind === SyntaxKind.JsxText) {
let text = getTextToEmit(<JsxText>children[i]);
if(text !== undefined) {
write(', "');
if (text !== undefined) {
write(", \"");
write(text);
write('"');
write("\"");
}
}
else {
Expand Down Expand Up @@ -1491,7 +1491,7 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
if (declaration.kind === SyntaxKind.ImportClause) {
// Identifier references default import
write(getGeneratedNameForNode(<ImportDeclaration>declaration.parent));
write(languageVersion === ScriptTarget.ES3 ? '["default"]' : ".default");
write(languageVersion === ScriptTarget.ES3 ? "[\"default\"]" : ".default");
return;
}
else if (declaration.kind === SyntaxKind.ImportSpecifier) {
Expand Down Expand Up @@ -4255,11 +4255,12 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
emitDetachedComments(ctor.body.statements);
}
emitCaptureThisForNodeIfNecessary(node);
let superCall: ExpressionStatement;
if (ctor) {
emitDefaultValueAssignments(ctor);
emitRestParameter(ctor);
if (baseTypeElement) {
var superCall = findInitialSuperCall(ctor);
superCall = findInitialSuperCall(ctor);
if (superCall) {
writeLine();
emit(superCall);
Expand Down Expand Up @@ -4936,7 +4937,7 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
let temp = createAndRecordTempVariable(TempFlags.Auto);
write("(typeof (");
emitNodeWithoutSourceMap(temp);
write(" = ")
write(" = ");
emitEntityNameAsExpression(typeName, /*useFallback*/ true);
write(") === 'function' && ");
emitNodeWithoutSourceMap(temp);
Expand Down Expand Up @@ -4995,7 +4996,7 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
//
// For the rules on serializing the type of each parameter declaration, see `serializeTypeOfDeclaration`.
if (node) {
var valueDeclaration: FunctionLikeDeclaration;
let valueDeclaration: FunctionLikeDeclaration;
if (node.kind === SyntaxKind.ClassDeclaration) {
valueDeclaration = getFirstConstructorWithBody(<ClassDeclaration>node);
}
Expand All @@ -5004,16 +5005,16 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
}

if (valueDeclaration) {
var parameters = valueDeclaration.parameters;
var parameterCount = parameters.length;
const parameters = valueDeclaration.parameters;
const parameterCount = parameters.length;
if (parameterCount > 0) {
for (var i = 0; i < parameterCount; i++) {
if (i > 0) {
write(", ");
}

if (parameters[i].dotDotDotToken) {
var parameterType = parameters[i].type;
let parameterType = parameters[i].type;
if (parameterType.kind === SyntaxKind.ArrayType) {
parameterType = (<ArrayTypeNode>parameterType).elementType;
}
Expand Down Expand Up @@ -5825,7 +5826,7 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
writeLine();
write("}");
writeLine();
write(`${exportFunctionForFile}(exports);`)
write(`${exportFunctionForFile}(exports);`);
decreaseIndent();
writeLine();
write("}");
Expand Down Expand Up @@ -6173,7 +6174,7 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
// exports_(reexports);
let reexportsVariableName = makeUniqueName("reexports");
writeLine();
write(`var ${reexportsVariableName} = {};`)
write(`var ${reexportsVariableName} = {};`);
writeLine();
for (let e of (<ExportDeclaration>importNode).exportClause.elements) {
write(`${reexportsVariableName}["`);
Expand Down Expand Up @@ -6439,7 +6440,7 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
if (isLineBreak(c)) {
if (firstNonWhitespace !== -1 && (lastNonWhitespace - firstNonWhitespace + 1 > 0)) {
let part = text.substr(firstNonWhitespace, lastNonWhitespace - firstNonWhitespace + 1);
result = (result ? result + '" + \' \' + "' : '') + part;
result = (result ? result + "\" + ' ' + \"" : "") + part;
}
firstNonWhitespace = -1;
}
Expand All @@ -6452,7 +6453,7 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
}
if (firstNonWhitespace !== -1) {
let part = text.substr(firstNonWhitespace);
result = (result ? result + '" + \' \' + "' : '') + part;
result = (result ? result + "\" + ' ' + \"" : "") + part;
}

return result;
Expand All @@ -6477,9 +6478,9 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
function emitJsxText(node: JsxText) {
switch (compilerOptions.jsx) {
case JsxEmit.React:
write('"');
write("\"");
write(trimReactWhitespace(node));
write('"');
write("\"");
break;

case JsxEmit.Preserve:
Expand All @@ -6494,9 +6495,9 @@ var __awaiter = (this && this.__awaiter) || function (thisArg, _arguments, Promi
switch (compilerOptions.jsx) {
case JsxEmit.Preserve:
default:
write('{');
write("{");
emit(node.expression);
write('}');
write("}");
break;
case JsxEmit.React:
emit(node.expression);
Expand Down
Loading