Suppress brace completion of Quotes in Comments by aozgaa · Pull Request #16433 · microsoft/TypeScript

aozgaa · 2017-06-10T04:12:56Z

Fixes https://developercommunity.visualstudio.com/content/problem/16657/javascript-comment-typing-a-single-quotation-mark.html

amcasey

Looks correct - comments are just about consistency and simplification.

amcasey · 2017-06-12T17:08:17Z

+    export function getRangeOfEnclosingComment(sourceFile: SourceFile, position: number, onlyMultiLine: boolean): CommentRange | undefined {
+        const precedingToken = findPrecedingToken(position, sourceFile);
+        const trailingRangesOfPreviousToken = precedingToken && getTrailingCommentRanges(sourceFile.text, precedingToken.end);
+        const leadingCommentRangesOfNextToken = getLeadingCommentRangesOfNode(getTokenAtPosition(sourceFile, position, /*includeJsDocComment*/ false), sourceFile);


Consider pulling out "followingToken" for consistency with "precedingToken".

Might also want to make specification of /includeJsDocComment/ consistent (or explicitly inconsistent).

amcasey · 2017-06-12T17:09:15Z


+    export function getRangeOfEnclosingComment(sourceFile: SourceFile, position: number, onlyMultiLine: boolean): CommentRange | undefined {
+        const precedingToken = findPrecedingToken(position, sourceFile);
+        const trailingRangesOfPreviousToken = precedingToken && getTrailingCommentRanges(sourceFile.text, precedingToken.end);


If this were set to empty, rather than undefined, in the absence of precedingToken, the checks below could be simplified.

findPrecedingToken looks like it might be quite expensive. We should verify any impact on typing before checking this in - especially as some brace matching operations happen as a blocking operation on the UI thread.

amcasey · 2017-06-12T17:11:17Z

+        if (commentRanges) {
+            for (const range of commentRanges) {
+                // We need to extend the range when in an unclosed multi-line comment.
+                if (range.pos < position && position < range.end ||


range.pos <= position? It should be impossible, but it seems strange to ignore the first character.

amcasey · 2017-06-12T17:14:16Z

+                // We need to extend the range when in an unclosed multi-line comment.
+                if (range.pos < position && position < range.end ||
+                    position === range.end && (range.kind === SyntaxKind.SingleLineCommentTrivia || position === sourceFile.getFullWidth())) {
+                    return onlyMultiLine && range.kind !== SyntaxKind.MultiLineCommentTrivia ? undefined : range;


Seems strange to accept and then reject single line comments. Another way to check would be to change range.kind === SyntaxKind.SingleLineCommentTrivia to (!onlyMultiLine && range.kind === SyntaxKind.SingleLineCommentTrivia).

amcasey · 2017-06-12T17:16:32Z

            }

+            switch (openingBrace) {
+                case CharacterCodes.singleQuote:


Which brace completions do we still provide and why?

Indeed. I've never liked any completions when typing inside a comment (including braces). Is there a rationale for what we do provide and what we don't?

This behavior mirrors that of CSharp editing in VS. But we can just as easily suppress all completions in comments.

amcasey · 2017-06-12T17:18:24Z

+                case CharacterCodes.singleQuote:
+                case CharacterCodes.doubleQuote:
+                case CharacterCodes.backtick:
+                    return !ts.formatting.getRangeOfEnclosingComment(sourceFile, position, /*onlyMultiLine*/ false);


The only caller passes literal false for the last argument and ignores the return value?

amcasey · 2017-06-12T17:19:36Z


 goTo.marker('1');
 verify.isValidBraceCompletionAtPosition('(');
+verify.not.isValidBraceCompletionAtPosition('"');


Would it be worthwhile to test a position that isn't at EOL (e.g. a contraction, as in the bug report)?

mhegazy · 2017-06-12T17:53:31Z

        }
    }

+    export function getRangeOfEnclosingComment(sourceFile: SourceFile, position: number, onlyMultiLine: boolean): CommentRange | undefined {


why not use isInComment in services\utilities?

billti · 2017-06-12T18:03:39Z

        }
    }

+    export function getRangeOfEnclosingComment(sourceFile: SourceFile, position: number, onlyMultiLine: boolean): CommentRange | undefined {


Why do we need this function? There's already an isInComment function exposed, and you don't use the range returned but just test if one was returned. Seems all we should care about is if the cursor is in a comment, right?

billti

It looks so easy now ;-)

aozgaa · 2017-06-13T18:07:47Z

Re-ran Jenkins test and it succeeded.

mhegazy · 2017-06-13T18:13:10Z

please port this to release-2.4

aozgaa · 2017-06-14T00:10:20Z

Double-checked the perf in VS via integrations tests which spammed brace opening characters, in and around comment blocks, in a 1000-line source file. For both completions tested, (parens and double quotes), performance remained virtually unchanged between the master branch with and without this PR applied. With the patch, results appeared marginally faster, but it was within a gut-check margin of error (difference of 1 additional keystroke out of 800 being above 30 ms).

Suppress brace completion of Quotes in Comments

798db1e

aozgaa requested review from amcasey, billti and mhegazy June 10, 2017 04:12

msftclas added the cla-not-required label Jun 10, 2017

amcasey approved these changes Jun 12, 2017

View reviewed changes

mhegazy reviewed Jun 12, 2017

View reviewed changes

billti reviewed Jun 12, 2017

View reviewed changes

respond to comments

70bace7

mhegazy approved these changes Jun 12, 2017

View reviewed changes

billti approved these changes Jun 13, 2017

View reviewed changes

aozgaa merged commit 2a05bb1 into master Jun 13, 2017

aozgaa deleted the suppressBraceCompletionOfQuotesinComments branch June 13, 2017 18:08

microsoft locked and limited conversation to collaborators Jun 14, 2018

Conversation

aozgaa commented Jun 10, 2017

Uh oh!

amcasey left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aozgaa Jun 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

billti left a comment

Choose a reason for hiding this comment

Uh oh!

aozgaa commented Jun 13, 2017

Uh oh!

mhegazy commented Jun 13, 2017

Uh oh!

aozgaa commented Jun 14, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

aozgaa Jun 12, 2017 •

edited

Loading