Skip to content

Fix ignore message indentation #22340

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
mhegazy merged 9 commits into from
Mar 6, 2018
Merged

Fix ignore message indentation #22340

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
e51d0a7
Update baselines for user tests
mhegazy Mar 1, 2018
b032bb3
Merge branch 'master' of https://github.com/Microsoft/TypeScript
mhegazy Mar 2, 2018
ffc9c89
Merge branch 'master' of https://github.com/Microsoft/TypeScript
mhegazy Mar 3, 2018
1b5629b
Add explicit indentation
mhegazy Mar 5, 2018
2e5bf36
Fix https://github.com/Microsoft/TypeScript/issues/21355: Format `// …
mhegazy Mar 5, 2018
9a8705b
Extract check to a separate function
mhegazy Mar 5, 2018
81e6b84
Consolidate checking for valid insert location
mhegazy Mar 5, 2018
f490227
Code review comments
mhegazy Mar 6, 2018
acc0783
Do not return makeChange
mhegazy Mar 6, 2018
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
49 changes: 25 additions & 24 deletions src/services/codefixes/disableJsDiagnostics.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,17 +9,17 @@ namespace ts.codefix {
registerCodeFix({
errorCodes,
getCodeActions(context) {
const { sourceFile, program, span } = context;
const { sourceFile, program, span, host, formatContext } = context;

if (!isInJavaScriptFile(sourceFile) || !isCheckJsEnabledForFile(sourceFile, program.getCompilerOptions())) {
return undefined;
}

const newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options);
const newLineCharacter = getNewLineOrDefaultFromHost(host, formatContext.options);

return [{
description: getLocaleSpecificMessage(Diagnostics.Ignore_this_error_message),
changes: [createFileTextChanges(sourceFile.fileName, [getIgnoreCommentLocationForLocation(sourceFile, span.start, newLineCharacter).change])],
changes: textChanges.ChangeTracker.with(context, t => makeChange(t, sourceFile, span.start, newLineCharacter)),
fixId,
},
{
Expand All @@ -33,36 +33,37 @@ namespace ts.codefix {
},
fixIds: [fixId],
getAllCodeActions: context => {
const seenLines = createMap<true>(); // Only need to add `// @ts-ignore` for a line once.
return codeFixAllWithTextChanges(context, errorCodes, (changes, err) => {
if (err.start !== undefined) {
const { lineNumber, change } = getIgnoreCommentLocationForLocation(err.file!, err.start, getNewLineOrDefaultFromHost(context.host, context.formatContext.options));
if (addToSeen(seenLines, lineNumber)) {
changes.push(change);
}
}
});
const newLineCharacter = getNewLineOrDefaultFromHost(context.host, context.formatContext.options);
const seenLines = createMap<true>();
return codeFixAll(context, errorCodes, (changes, diag) => makeChange(changes, diag.file!, diag.start!, newLineCharacter, seenLines));
},
});

function getIgnoreCommentLocationForLocation(sourceFile: SourceFile, position: number, newLineCharacter: string): { lineNumber: number, change: TextChange } {
function makeChange(changes: textChanges.ChangeTracker, sourceFile: SourceFile, position: number, newLineCharacter: string, seenLines?: Map<true>) {
Copy link

@ghost ghost Mar 5, 2018

Choose a reason for hiding this comment

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

It would be neater if changes handled the newlines itself -- see discussion at #20574.

Copy link
Contributor Author

@mhegazy mhegazy Mar 5, 2018

Choose a reason for hiding this comment

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

it is the only place where we do something like this with comments. i did not make much sense to put it in the tracker.

Copy link

@ghost ghost Mar 5, 2018 •
edited by ghost
Loading

Choose a reason for hiding this comment

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

You could also make changes.newLineCharacter public to avoid computing it yourself. There's no particular reason for it to be private if you want to handle newlines yourself in this use.

if (isInComment(sourceFile, position) || isInString(sourceFile, position) || isInTemplateString(sourceFile, position)) {
Copy link

@ghost ghost Mar 5, 2018

Choose a reason for hiding this comment

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

When we call makeChange we're about to unconditionally return the codefix -- so this would advertise the fix as available and do nothing?

Copy link
Contributor Author

@mhegazy mhegazy Mar 5, 2018

Choose a reason for hiding this comment

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

this is for the other code path for multiple entries.. i can split it.

Copy link
Contributor Author

@mhegazy mhegazy Mar 5, 2018

Choose a reason for hiding this comment

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

and we already had a test for it.

return;
}

const { line: lineNumber } = getLineAndCharacterOfPosition(sourceFile, position);

// Only need to add `// @ts-ignore` for a line once.
if (seenLines && !addToSeen(seenLines, lineNumber)) {
return;
}

const lineStartPosition = getStartPositionOfLine(lineNumber, sourceFile);
const startPosition = getFirstNonSpaceCharacterPosition(sourceFile.text, lineStartPosition);

// First try to see if we can put the '// @ts-ignore' on the previous line.
// We need to make sure that we are not in the middle of a string literal or a comment.
// We also want to check if the previous line holds a comment for a node on the next line
// if so, we do not want to separate the node from its comment if we can.
if (!isInComment(sourceFile, startPosition) && !isInString(sourceFile, startPosition) && !isInTemplateString(sourceFile, startPosition)) {
const token = getTouchingToken(sourceFile, startPosition, /*includeJsDocComment*/ false);
const tokenLeadingComments = getLeadingCommentRangesOfNode(token, sourceFile);
if (!tokenLeadingComments || !tokenLeadingComments.length || tokenLeadingComments[0].pos >= startPosition) {
return { lineNumber, change: createTextChangeFromStartLength(startPosition, 0, `// @ts-ignore${newLineCharacter}`) };
}
}
// If so, we do not want to separate the node from its comment if we can.
// Otherwise, add an extra new line immediately before the error span.
const insertAtLineStart = !isInComment(sourceFile, startPosition) &&
Copy link

@ghost ghost Mar 5, 2018

Choose a reason for hiding this comment

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

Just checked for this at top, so should always be true?

Copy link
Contributor Author

@mhegazy mhegazy Mar 5, 2018

Choose a reason for hiding this comment

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

that is a different position

Copy link

@ghost ghost Mar 5, 2018

Choose a reason for hiding this comment

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

OK. Could this line be isValidSuppressLocation(sourceFile, startPosition);?

Copy link
Contributor Author

@mhegazy mhegazy Mar 5, 2018

Choose a reason for hiding this comment

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

done.

!isInString(sourceFile, startPosition) && !isInTemplateString(sourceFile, startPosition);

// If all fails, add an extra new line immediately before the error span.
return { lineNumber, change: createTextChangeFromStartLength(position, 0, `${position === startPosition ? "" : newLineCharacter}// @ts-ignore${newLineCharacter}`) };
const token = getTouchingToken(sourceFile, insertAtLineStart ? startPosition : position, /*includeJsDocComment*/ false);
const clone = setStartsOnNewLine(getSynthesizedDeepClone(token), true);
addSyntheticLeadingComment(clone, SyntaxKind.SingleLineCommentTrivia, " @ts-ignore");
changes.replaceNode(sourceFile, token, clone, { preseveLeadingWhiteSpaces: true, prefix: insertAtLineStart ? undefined : newLineCharacter });
}
}
6 changes: 5 additions & 1 deletion src/services/textChanges.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,10 @@ namespace ts.textChanges {
* Text of inserted node will be formatted with this delta, otherwise delta will be inferred from the new node kind
*/
delta?: number;
/**
* Do not trim leading white spaces in the edit range
*/
preseveLeadingWhiteSpaces?: boolean;
Copy link
Member

@DanielRosenwasser DanielRosenwasser Mar 6, 2018

Choose a reason for hiding this comment

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

preserveLeadingWhitespace

}

enum ChangeKind {
Expand Down Expand Up @@ -628,7 +632,7 @@ namespace ts.textChanges {
? change.nodes.map(n => removeSuffix(format(n), newLineCharacter)).join(newLineCharacter)
: format(change.node);
// strip initial indentation (spaces or tabs) if text will be inserted in the middle of the line
const noIndent = (options.indentation !== undefined || getLineStartPositionForPosition(pos, sourceFile) === pos) ? text : text.replace(/^\s+/, "");
const noIndent = (options.preseveLeadingWhiteSpaces || options.indentation !== undefined || getLineStartPositionForPosition(pos, sourceFile) === pos) ? text : text.replace(/^\s+/, "");
return (options.prefix || "") + noIndent + (options.suffix || "");
}

Expand Down
8 changes: 4 additions & 4 deletions tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile6.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@
// @Filename: a.js
////var x = 0;
////
////function f(_a) {
//// [|f(x());|]
////}
////function f(_a) {[|
//// f(x());
////|]}

// Disable checking for next line
verify.rangeAfterCodeFix(`// @ts-ignore
verify.rangeAfterCodeFix(` // @ts-ignore
f(x());`, /*includeWhiteSpace*/ false, /*errorCode*/ undefined, /*index*/ 0);

8 changes: 4 additions & 4 deletions tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile7.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@
// @Filename: a.js
////var x = 0;
////
////function f(_a) {
//// [|x();|]
////}
////function f(_a) {[|
//// x();
////|]}

// Disable checking for next line
verify.rangeAfterCodeFix(`// @ts-ignore
verify.rangeAfterCodeFix(`// @ts-ignore
x();`, /*includeWhiteSpace*/ false, /*errorCode*/ undefined, /*index*/ 0);

10 changes: 5 additions & 5 deletions tests/cases/fourslash/codeFixDisableJsDiagnosticsInFile8.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,13 +7,13 @@
// @Filename: a.js
////var x = 0;
////
////function f(_a) {
////function f(_a) {[|
//// /** comment for f */
//// [|f(x());|]
////}
//// f(x());
////|]}

// Disable checking for next line
verify.rangeAfterCodeFix(`f(
verify.rangeAfterCodeFix(` /** comment for f */
// @ts-ignore
x());`, /*includeWhiteSpace*/ false, /*errorCode*/ undefined, /*index*/ 0);
f(x());`, /*includeWhiteSpace*/ false, /*errorCode*/ undefined, /*index*/ 0);