Add some validation to ensure property setters/getters are documented correctly

octogonz · octogonz · commit a07e853d2e08 · 2019-11-29T07:24:59.000-08:00
diff --git a/apps/api-extractor/src/api/ExtractorMessageId.ts b/apps/api-extractor/src/api/ExtractorMessageId.ts
@@ -85,7 +85,17 @@ export const enum ExtractorMessageId {
   /**
    * "The `@link` reference could not be resolved".
    */
-  UnresolvedLink = 'ae-unresolved-link'
+  UnresolvedLink = 'ae-unresolved-link',
+
+  /**
+   * "The doc comment for the property ___ must appear on the getter, not the setter.".
+   */
+  SetterWithDocs = 'ae-setter-with-docs',
+
+  /**
+   * "The property ___ has a setter but no getter.".
+   */
+  MissingGetter = 'ae-missing-getter',
 }
 
 export const allExtractorMessageIds: Set<string> = new Set<string>([
@@ -102,5 +112,7 @@ export const allExtractorMessageIds: Set<string> = new Set<string>([
   'ae-unresolved-inheritdoc-reference',
   'ae-unresolved-inheritdoc-base',
   'ae-cyclic-inherit-doc',
-  'ae-unresolved-link'
+  'ae-unresolved-link',
+  'ae-setter-with-docs',
+  'ae-missing-getter'
 ]);
diff --git a/apps/api-extractor/src/collector/Collector.ts b/apps/api-extractor/src/collector/Collector.ts
@@ -528,14 +528,24 @@ export class Collector {
 
       // For a getter/setter pair, make the setter ancillary to the getter
       if (astDeclaration.declaration.kind === ts.SyntaxKind.SetAccessor) {
+        let foundGetter: boolean = false;
         for (const getterAstDeclaration of astDeclaration.astSymbol.astDeclarations) {
           if (getterAstDeclaration.declaration.kind === ts.SyntaxKind.GetAccessor) {
             const getterMetadata: DeclarationMetadata = getterAstDeclaration.metadata as DeclarationMetadata;
 
             // Associate it with the getter
             getterMetadata.addAncillaryDeclaration(astDeclaration);
+
+            foundGetter = true;
           }
         }
+
+        if (!foundGetter) {
+          this.messageRouter.addAnalyzerIssue(
+            ExtractorMessageId.MissingGetter,
+            `The property "${astDeclaration.astSymbol.localName}" has a setter but no getter.`,
+            astDeclaration);
+        }
       }
     }
 
diff --git a/apps/api-extractor/src/enhancers/DocCommentEnhancer.ts b/apps/api-extractor/src/enhancers/DocCommentEnhancer.ts
@@ -127,8 +127,20 @@ export class DocCommentEnhancer {
         );
 
       }
+      return;
+    }
+
+    if (astDeclaration.declaration.kind === ts.SyntaxKind.SetAccessor) {
+      if (metadata.tsdocComment) {
+        this._collector.messageRouter.addAnalyzerIssue(ExtractorMessageId.SetterWithDocs,
+          `The doc comment for the property "${astDeclaration.astSymbol.localName}"`
+          + ` must appear on the getter, not the setter.`,
+          astDeclaration);
+      }
+      return;
+    }
 
-    } else if (metadata.tsdocComment) {
+    if (metadata.tsdocComment) {
       // Require the summary to contain at least 10 non-spacing characters
       metadata.needsDocumentation = !tsdoc.PlainTextEmitter.hasAnyTextContent(
         metadata.tsdocComment.summarySection, 10);
diff --git a/apps/api-extractor/src/generators/ApiReportGenerator.ts b/apps/api-extractor/src/generators/ApiReportGenerator.ts
@@ -404,7 +404,8 @@ export class ApiReportGenerator {
       }
     }
 
-    if (declarationMetadata.needsDocumentation) {
+    // Note that ancillary declarations aren't supposed to have documentation
+    if (declarationMetadata.needsDocumentation && !declarationMetadata.isAncillary) {
       footerParts.push('(undocumented)');
     }
 
diff --git a/common/reviews/api/api-extractor-model.api.md b/common/reviews/api/api-extractor-model.api.md
@@ -627,9 +627,7 @@ export class ExcerptToken {
 
 // @public (undocumented)
 export const enum ExcerptTokenKind {
-    // (undocumented)
     Content = "Content",
-    // (undocumented)
     Reference = "Reference"
 }
 
diff --git a/common/reviews/api/api-extractor.api.md b/common/reviews/api/api-extractor.api.md
@@ -119,9 +119,11 @@ export const enum ExtractorMessageId {
     InternalMissingUnderscore = "ae-internal-missing-underscore",
     InternalMixedReleaseTag = "ae-internal-mixed-release-tag",
     MisplacedPackageTag = "ae-misplaced-package-tag",
+    MissingGetter = "ae-missing-getter",
     MissingReleaseTag = "ae-missing-release-tag",
     PreapprovedBadReleaseTag = "ae-preapproved-bad-release-tag",
     PreapprovedUnsupportedType = "ae-preapproved-unsupported-type",
+    SetterWithDocs = "ae-setter-with-docs",
     UnresolvedInheritDocBase = "ae-unresolved-inheritdoc-base",
     UnresolvedInheritDocReference = "ae-unresolved-inheritdoc-reference",
     UnresolvedLink = "ae-unresolved-link"

Original file line number	Diff line number	Diff line change
`@@ -528,14 +528,24 @@ export class Collector {`
`528`	`528`
`529`	`529`	`// For a getter/setter pair, make the setter ancillary to the getter`
`530`	`530`	`if (astDeclaration.declaration.kind === ts.SyntaxKind.SetAccessor) {`
	`531`	`+ let foundGetter: boolean = false;`
`531`	`532`	`for (const getterAstDeclaration of astDeclaration.astSymbol.astDeclarations) {`
`532`	`533`	`if (getterAstDeclaration.declaration.kind === ts.SyntaxKind.GetAccessor) {`
`533`	`534`	`const getterMetadata: DeclarationMetadata = getterAstDeclaration.metadata as DeclarationMetadata;`
`534`	`535`
`535`	`536`	`// Associate it with the getter`
`536`	`537`	`getterMetadata.addAncillaryDeclaration(astDeclaration);`
	`538`	`+`
	`539`	`+ foundGetter = true;`
`537`	`540`	`}`
`538`	`541`	`}`
	`542`	`+`
	`543`	`+ if (!foundGetter) {`
	`544`	`+ this.messageRouter.addAnalyzerIssue(`
	`545`	`+ ExtractorMessageId.MissingGetter,`
	`546`	+ `The property "${astDeclaration.astSymbol.localName}" has a setter but no getter.`,
	`547`	`+ astDeclaration);`
	`548`	`+ }`
`539`	`549`	`}`
`540`	`550`	`}`
`541`	`551`
Original file line number	Diff line number	Diff line change
`@@ -404,7 +404,8 @@ export class ApiReportGenerator {`
`404`	`404`	`}`
`405`	`405`	`}`
`406`	`406`
`407`		`- if (declarationMetadata.needsDocumentation) {`
	`407`	`+ // Note that ancillary declarations aren't supposed to have documentation`
	`408`	`+ if (declarationMetadata.needsDocumentation && !declarationMetadata.isAncillary) {`
`408`	`409`	`footerParts.push('(undocumented)');`
`409`	`410`	`}`
`410`	`411`
Original file line number	Diff line number	Diff line change
`@@ -627,9 +627,7 @@ export class ExcerptToken {`
`627`	`627`
`628`	`628`	`// @public (undocumented)`
`629`	`629`	`export const enum ExcerptTokenKind {`
`630`		`- // (undocumented)`
`631`	`630`	`Content = "Content",`
`632`		`- // (undocumented)`
`633`	`631`	`Reference = "Reference"`
`634`	`632`	`}`
`635`	`633`