Insert JsDoc comment templates for additional nodes #19544

uniqueiniquity · 2017-10-27T23:59:14Z

In response to https://developercommunity.visualstudio.com/content/problem/138023/typescript-editor-jsdoc-comments-not-auto-generate.html.

Previously, we were not inserting jsDoc comment templates before interface declarations, method signatures, property signatures, and enums, whereas we were for class declarations and method declarations. This adds this functionality in.

Also, following a suggestion by @Andy-MS, we no longer return undefined if it is inappropriate to insert a doc comment template. Instead, we return an empty template.

ghost · 2017-10-30T15:01:03Z

tests/cases/fourslash/docCommentTemplateInterfaces.ts

+ * 
+ */`);
+
+verify.emptyDocCommentTemplateAt("propertybar");


Why not give us the above template in this case too?

I can, it was just outside of the scope of this particular bug fix. I can go and add these too if we want.

I think we could just return that template (with the current indentation) at any arbitrary location where we don't have specific handling.

Any arbitrary location? I feel like it should always be leading up to a declaration or member of some sort.

ghost · 2017-10-30T15:45:42Z

src/services/jsDoc.ts

                    return { commentOwner, parameters };

                case SyntaxKind.ClassDeclaration:
+                case SyntaxKind.InterfaceDeclaration:


Looks like commentOwner is only used in one place: if (commentOwner.getStart() < position) { return undefined; }.
Seems like this function could just be getParameters, but don't return parameters if you're inside the function, only if you're on top of it.

I like this current setup since we have an explicit list of what places are permissible for these templates, and it's easy to add new things.

If a user types /** in an empty file and hits enter, I would expect to see an empty doc comment -- it's unambiguous that that's what they're trying to do. No sense in not giving the user formatting just because they're not above a declaration. An explicit list of places where we allow doc comments would just lead to users frustrated over why sometimes starting a comment works and sometimes it doesn't.

Even if they're in the middle of an expression? For example, what if someone's adding a comment to a parameter (foo(/*param*/ undefined)) and then accidentally hit * twice. I guess they can just hit undo but it seems easy to trigger.

Do we fill in the comment immediately on the second * or do we wait for them to hit enter?

In vscode you must hit enter. What if a user wants to add a single-line comment to an interface?

I mean same as my description above: if they accidentally hit a second star, then it will trigger, but otherwise it won't. Seems somehow less bad then triggering in the middle of a parenthetical block.

I mean a single-line jsdoc comment. These seem pretty common.

/** My interface */ interface I { /** My method */ m(): void; }

Huh. Yeah we never use that with the current implementation.

So maybe we should just autocomplete to empty, one-line jsDoc comments and then if there are params, autocomplete to a multi-line and fill in the params?

mhegazy · 2017-10-30T22:19:15Z

src/services/jsDoc.ts

        // Check if in a context where we don't want to perform any insertion
        if (isInString(sourceFile, position) || isInComment(sourceFile, position) || hasDocComment(sourceFile, position)) {
-            return undefined;
+            return emptyDocComment;


so why do we return an empty object here? what is wrong in undefined?

Was @Andy-MS's suggestion. Andy, could you elaborate?

See #19544 (comment)

aha, I misunderstood what you meant by empty doc comment template.

so how is the empty file case related to isInString or isIncomment ?

if you want to enable them all the time that is fine, but not in strings and not in comments

mhegazy · 2017-10-30T22:21:25Z

src/services/jsDoc.ts

+                case SyntaxKind.InterfaceDeclaration:
+                case SyntaxKind.PropertySignature:
+                case SyntaxKind.EnumDeclaration:
+                case SyntaxKind.EnumMember:


type alias as well.

mhegazy · 2017-10-30T22:22:34Z

src/services/jsDoc.ts

@@ -255,19 +259,21 @@ namespace ts.JsDoc {
    }
    function getCommentOwnerInfo(tokenAtPos: Node): CommentOwnerInfo | undefined {
        // TODO: add support for:
-        // - enums/enum members
-        // - interfaces
-        // - property declarations
        // - potentially property assignments


do not think this is comment is needed either. we handle these in BinaryExpression section.

This reverts commit 22eb519.

This reverts commit b566480.

uniqueiniquity · 2017-11-03T17:03:29Z

I got rid of the empty template approach. Turns out it makes VS behave weirdly, whereas undefined induces the intended behavior.

uniqueiniquity · 2017-11-03T17:05:30Z

This function will now:

return a single-line JsDoc comment unless there are parameters to document, in which case it will fill them in as before.
autocomplete everywhere except for inside strings, comments, template literals, regex literals, and JSXText

ghost · 2017-11-03T17:31:24Z

src/services/jsDoc.ts

+            return singleLineTemplate;
+        }
+
+        if (parameters.length === 0) {


Nit: Use || to combine with above

ghost · 2017-11-03T17:39:24Z

src/services/jsDoc.ts

-                }
+        for (let i = 0; i < parameters.length; i++) {
+            const currentName = parameters[i].name;
+            const paramName = currentName.kind === SyntaxKind.Identifier ?


ghost · 2017-11-03T17:41:01Z

tests/cases/fourslash/docCommentTemplateClassDeclMethods01.ts

@@ -1,7 +1,7 @@
 /// <reference path='fourslash.ts' />

 const enum Indentation {
-    Standard = 8,
+    Standard = 3,


Why this change? It's unusual to set indentation to 3.

Indentation is actually a bad name for it. It's actually the caret offset into the template. But indentation is the term consistently used for it. I could go in and change it all to Offset if warranted.

Maybe in the old version this actually was an indentation?
This really makes no sense as an enum and could just be consts.

uniqueiniquity added 4 commits October 27, 2017 16:56

Return empty doc comment instead of undefined

22eb519

Update tests to expect empty doc comment template

b566480

Return doc comment template for interfaces and method signatures

7aeb11b

Update comments

4977218

uniqueiniquity requested review from aozgaa and a user October 27, 2017 23:59

ghost reviewed Oct 30, 2017

View reviewed changes

Add support for enums and property signatures

976c25c

uniqueiniquity changed the title ~~Insert JsDoc comment templates for interfaces and method signatures~~ Insert JsDoc comment templates for additional nodes Oct 30, 2017

mhegazy reviewed Oct 30, 2017

View reviewed changes

uniqueiniquity added 4 commits November 2, 2017 09:55

Complete to single line jsdoc comment if no params

509b9ad

Revert "Return empty doc comment instead of undefined"

12baae6

This reverts commit 22eb519.

Revert "Update tests to expect empty doc comment template"

b17b7b9

This reverts commit b566480.

Add undefined to return type

b1b611f

microsoft deleted a comment Nov 2, 2017

Loosen restrictions on jsdoc completion locations

612616a

ghost approved these changes Nov 3, 2017

View reviewed changes

uniqueiniquity added 2 commits November 3, 2017 11:19

Respond to CR

2109350

Rename offsets in tests

d2114e1

uniqueiniquity merged commit 4977bf4 into microsoft:master Nov 6, 2017

uniqueiniquity deleted the interfaceJsDoc branch November 6, 2017 17:50

ghost mentioned this pull request Nov 17, 2017

docCommentTemplate does not return newlines in empty template anymore #20089

Closed

uniqueiniquity mentioned this pull request Dec 12, 2017

Revert to inclusion list for JsDoc comment completion #20640

Merged

microsoft locked and limited conversation to collaborators Jun 14, 2018

Insert JsDoc comment templates for additional nodes #19544

Insert JsDoc comment templates for additional nodes #19544

Uh oh!

Conversation

uniqueiniquity commented Oct 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

ghost Oct 30, 2017 • edited by ghost 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!

ghost Oct 30, 2017 • edited by ghost 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

uniqueiniquity commented Nov 3, 2017

Uh oh!

uniqueiniquity commented Nov 3, 2017

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!

uniqueiniquity Nov 3, 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!

Uh oh!

uniqueiniquity commented Oct 27, 2017 •

edited

Loading

ghost Oct 30, 2017 •

edited by ghost

Loading

ghost Oct 30, 2017 •

edited by ghost

Loading

uniqueiniquity Nov 3, 2017 •

edited

Loading