infra: improve error messages for parameter properties (#3082)

faker-js · Sep 9, 2024 · acb8b52 · acb8b52
1 parent f128d77
commit acb8b52
Show file tree

Hide file tree

Showing 6 changed files with 99 additions and 51 deletions.
diff --git a/scripts/apidocs/processing/error.ts b/scripts/apidocs/processing/error.ts
@@ -1,19 +1,28 @@
 import { FakerError } from '../../../src/errors/faker-error';
+import { CI_PREFLIGHT } from '../../env';
 import type { SourceableNode } from './source';
 import { getSourcePath } from './source';
 
 export class FakerApiDocsProcessingError extends FakerError {
   constructor(options: {
     type: string;
     name: string;
-    source: string | SourceableNode;
+    source: SourceableNode;
     cause: unknown;
   }) {
     const { type, name, source, cause } = options;
-    const sourceText =
-      typeof source === 'string' ? source : getSourcePathText(source);
+
+    const mainText = `Failed to process ${type} '${name}'`;
     const causeText = cause instanceof Error ? cause.message : '';
-    super(`Failed to process ${type} ${name} at ${sourceText} : ${causeText}`, {
+    const { filePath, line, column } = getSourcePath(source);
+    const sourceText = `${filePath}:${line}:${column}`;
+
+    if (CI_PREFLIGHT) {
+      const sourceArgs = `file=${filePath},line=${line},col=${column}`;
+      console.log(`::error ${sourceArgs}::${mainText}: ${causeText}`);
+    }
+
+    super(`${mainText} at ${sourceText} : ${causeText}`, {
       cause,
     });
   }
@@ -22,7 +31,7 @@ export class FakerApiDocsProcessingError extends FakerError {
 export function newProcessingError(options: {
   type: string;
   name: string;
-  source: string | SourceableNode;
+  source: SourceableNode;
   cause: unknown;
 }): FakerApiDocsProcessingError {
   const { cause } = options;
@@ -33,8 +42,3 @@ export function newProcessingError(options: {
 
   return new FakerApiDocsProcessingError(options);
 }
-
-function getSourcePathText(source: SourceableNode): string {
-  const { filePath, line, column } = getSourcePath(source);
-  return `${filePath}:${line}:${column}`;
-}
diff --git a/scripts/apidocs/processing/jsdocs.ts b/scripts/apidocs/processing/jsdocs.ts
@@ -10,7 +10,11 @@ import {
 export type JSDocableLikeNode = Pick<JSDocableNode, 'getJsDocs'>;
 
 export function getJsDocs(node: JSDocableLikeNode): JSDoc {
-  return exactlyOne(node.getJsDocs(), 'jsdocs');
+  return exactlyOne(
+    node.getJsDocs(),
+    'jsdocs',
+    'Please ensure that each method signature has JSDocs, and that all properties of option/object parameters are documented with both @param tags and inline JSDocs.'
+  );
 }
 
 export function getDeprecated(jsdocs: JSDoc): string | undefined {

diff --git a/scripts/apidocs/processing/parameter.ts b/scripts/apidocs/processing/parameter.ts
@@ -1,5 +1,6 @@
 import type {
   PropertySignature,
+  Symbol,
   Type,
   TypeParameterDeclaration,
 } from 'ts-morph';
@@ -174,30 +175,43 @@ function processComplexParameter(
     return type
       .getApparentProperties()
       .flatMap((parameter) => {
-        const declaration = exactlyOne(
-          parameter.getDeclarations(),
-          'property declaration'
-        ) as PropertySignature;
-        const propertyType = declaration.getType();
-        const jsdocs = getJsDocs(declaration);
-        const deprecated = getDeprecated(jsdocs);
-
-        return [
-          {
-            name: `${name}.${parameter.getName()}${getNameSuffix(propertyType)}`,
-            type: getTypeText(propertyType, {
-              abbreviate: false,
-              stripUndefined: true,
-            }),
-            default: getDefault(jsdocs),
-            description:
-              getDescription(jsdocs) +
-              (deprecated ? `\n\n**DEPRECATED:** ${deprecated}` : ''),
-          },
-        ];
+        try {
+          return processComplexParameterProperty(name, parameter);
+        } catch (error) {
+          throw newProcessingError({
+            type: 'property',
+            name: `${name}.${parameter.getName()}`,
+            source: parameter.getDeclarations()[0],
+            cause: error,
+          });
+        }
       })
       .sort((a, b) => a.name.localeCompare(b.name));
   }
 
   return [];
 }
+
+function processComplexParameterProperty(name: string, parameter: Symbol) {
+  const declaration = exactlyOne(
+    parameter.getDeclarations(),
+    'property declaration'
+  ) as PropertySignature;
+  const propertyType = declaration.getType();
+  const jsdocs = getJsDocs(declaration);
+  const deprecated = getDeprecated(jsdocs);
+
+  return [
+    {
+      name: `${name}.${parameter.getName()}${getNameSuffix(propertyType)}`,
+      type: getTypeText(propertyType, {
+        abbreviate: false,
+        stripUndefined: true,
+      }),
+      default: getDefault(jsdocs),
+      description:
+        getDescription(jsdocs) +
+        (deprecated ? `\n\n**DEPRECATED:** ${deprecated}` : ''),
+    },
+  ];
+}
diff --git a/scripts/apidocs/utils/value-checks.ts b/scripts/apidocs/utils/value-checks.ts
@@ -1,7 +1,11 @@
-export function exactlyOne<T>(input: ReadonlyArray<T>, property: string): T {
+export function exactlyOne<T>(
+  input: ReadonlyArray<T>,
+  property: string,
+  extraDescription: string = ''
+): T {
   if (input.length !== 1) {
     throw new Error(
-      `Expected exactly one element for ${property}, got ${input.length}`
+      `Expected exactly one ${property} element, got ${input.length}. ${extraDescription}`
     );
   }
 
@@ -10,11 +14,12 @@ export function exactlyOne<T>(input: ReadonlyArray<T>, property: string): T {
 
 export function optionalOne<T>(
   input: ReadonlyArray<T>,
-  property: string
+  property: string,
+  extraDescription: string = ''
 ): T | undefined {
   if (input.length > 1) {
     throw new Error(
-      `Expected one optional element for ${property}, got ${input.length}`
+      `Expected one optional ${property} element, got ${input.length}. ${extraDescription}`
     );
   }
 
@@ -23,47 +28,66 @@ export function optionalOne<T>(
 
 export function required<T>(
   input: T | undefined,
-  property: string
+  property: string,
+  extraDescription: string = ''
 ): NonNullable<T> {
   if (input == null) {
-    throw new Error(`Expected a value for ${property}, got undefined`);
+    throw new Error(
+      `Expected a value for ${property}, got undefined. ${extraDescription}`
+    );
   }
 
   return input;
 }
 
 export function allRequired<T>(
   input: ReadonlyArray<T | undefined>,
-  property: string
+  property: string,
+  extraDescription: string = ''
 ): Array<NonNullable<T>> {
-  return input.map((v, i) => required(v, `${property}[${i}]`));
+  return input.map((v, i) =>
+    required(v, `${property}[${i}]`, extraDescription)
+  );
 }
 
 export function atLeastOne<T>(
   input: ReadonlyArray<T>,
-  property: string
+  property: string,
+  extraDescription: string = ''
 ): ReadonlyArray<T> {
   if (input.length === 0) {
-    throw new Error(`Expected at least one element for ${property}`);
+    throw new Error(
+      `Expected at least one ${property} element. ${extraDescription}`
+    );
   }
 
   return input;
 }
 
 export function atLeastOneAndAllRequired<T>(
   input: ReadonlyArray<T | undefined>,
-  property: string
+  property: string,
+  extraDescription: string = ''
 ): ReadonlyArray<NonNullable<T>> {
-  return atLeastOne(allRequired(input, property), property);
+  return atLeastOne(
+    allRequired(input, property, extraDescription),
+    property,
+    extraDescription
+  );
 }
 
-export function valueForKey<T>(input: Record<string, T>, key: string): T {
-  return required(input[key], key);
+export function valueForKey<T>(
+  input: Record<string, T>,
+  key: string,
+  extraDescription: string = ''
+): T {
+  return required(input[key], key, extraDescription);
 }
 
 export function valuesForKeys<T>(
   input: Record<string, T>,
-  keys: string[]
+  keys: string[],
+  extraDescription: string = ''
 ): T[] {
-  return keys.map((key) => valueForKey(input, key));
+  return keys.map((key) => valueForKey(input, key, extraDescription));
 }
diff --git a/scripts/env.ts b/scripts/env.ts
@@ -0,0 +1,3 @@
+import { env } from 'node:process';
+
+export const CI_PREFLIGHT = env.CI_PREFLIGHT === 'true';
diff --git a/vitest.config.ts b/vitest.config.ts
@@ -1,4 +1,5 @@
 import { defineConfig } from 'vitest/config';
+import { CI_PREFLIGHT } from './scripts/env';
 
 const VITEST_SEQUENCE_SEED = Date.now();
 
@@ -14,9 +15,7 @@ export default defineConfig({
       reporter: ['clover', 'cobertura', 'lcov', 'text'],
       include: ['src'],
     },
-    reporters: process.env.CI_PREFLIGHT
-      ? ['basic', 'github-actions']
-      : ['basic'],
+    reporters: CI_PREFLIGHT ? ['basic', 'github-actions'] : ['basic'],
     sequence: {
       seed: VITEST_SEQUENCE_SEED,
       shuffle: true,