Multiple clean up fixes for JSDoc and parameter handling (#557)

* Multiple clean up fixes for JSDoc and parameter handling * Add comment * Address PR Comments
Azure · Jan 31, 2020 · a8e07f3 · a8e07f3
1 parent e9b38f0
commit a8e07f3
Show file tree

Hide file tree

Showing 24 changed files with 305 additions and 301 deletions.
diff --git a/src/generators/clientContextFileGenerator.ts b/src/generators/clientContextFileGenerator.ts
@@ -14,11 +14,8 @@ import { ClientDetails } from "../models/clientDetails";
 import { PackageDetails } from "../models/packageDetails";
 import { ParameterDetails } from "../models/parameterDetails";
 import { ImplementationLocation, SchemaType } from "@azure-tools/codemodel";
-import {
-  getCredentialsCheck,
-  getCredentialsParameter
-} from "./utils/parameterUtils";
 import { BaseUrlDetails } from "../transforms/urlTransforms";
+import { formatJsDocParam } from "./utils/parameterUtils";
 
 export function generateClientContext(
   clientDetails: ClientDetails,
@@ -29,7 +26,6 @@ export function generateClientContext(
     param => param.implementationLocation === ImplementationLocation.Client
   );
   const requiredParams = getRequiredParameters(clientParams);
-  const hasCredentials = !!clientDetails.options.addCredentials;
   const clientContextClassName = `${clientDetails.className}Context`;
   const clientContextFileName = normalizeName(
     clientContextClassName,
@@ -52,20 +48,17 @@ export function generateClientContext(
 
   const classConstructor = buildConstructor(contextClass, {
     clientContextClassName,
-    hasCredentials,
     requiredParams
   });
 
   writeConstructorBody(classConstructor, {
     clientParams,
-    hasCredentials,
     clientDetails,
     requiredPaams: requiredParams
   });
 }
 
 interface WriteConstructorBodyParameters {
-  hasCredentials: boolean;
   requiredPaams: ParameterDetails[];
   clientParams: ParameterDetails[];
   clientDetails: ClientDetails;
@@ -84,8 +77,8 @@ function writePackageInfo(
 ) {
   sourceFile.addStatements([
     `\n\n`,
-    `const packageName = "${packageDetails.name}";`,
-    `const packageVersion = "${packageDetails.version}";`
+    `const packageName = "${packageDetails.name || ""}";`,
+    `const packageVersion = "${packageDetails.version || ""}";`
   ]);
 }
 
@@ -106,33 +99,23 @@ function writeClassProperties(
 
 function writeConstructorBody(
   classConstructor: ConstructorDeclaration,
-  {
-    clientParams,
-    hasCredentials,
-    requiredPaams,
-    clientDetails
-  }: WriteConstructorBodyParameters
+  { clientParams, requiredPaams, clientDetails }: WriteConstructorBodyParameters
 ) {
-  const optionalParams = getOptionalParameters(clientParams);
   const addBlankLine = true;
   const requiredParameters = getRequiredParamAssignments(requiredPaams);
   const constantParameters = getConstantClientParamAssignments(clientParams);
-  const optionalParameters = getOptionalParameterAssignments(optionalParams);
   classConstructor.addStatements([
-    writeStatement(getCredentialsCheck(hasCredentials)),
     writeStatements(getRequiredParamChecks(requiredPaams), addBlankLine),
-    writeStatement(writeDefaultOptions(hasCredentials)),
+    writeStatement(
+      writeDefaultOptions(clientParams.some(p => p.name === "credentials"))
+    ),
+    writeStatement(getBaseUriStatement(clientDetails.baseUrl), addBlankLine),
     requiredParameters.length ? "// Parameter assignments" : "",
     writeStatements(getRequiredParamAssignments(requiredPaams), addBlankLine),
     constantParameters.length
       ? "// Assigning values to Constant parameters"
       : "",
-    writeStatements(constantParameters, addBlankLine),
-    writeStatement(getBaseUriStatement(clientDetails.baseUrl), addBlankLine),
-    optionalParameters.length
-      ? "// Replacing parameter defaults with user-provided parameters."
-      : "",
-    writeStatements(optionalParameters)
+    writeStatements(constantParameters, addBlankLine)
   ]);
 }
 
@@ -180,25 +163,24 @@ function buildClass(sourceFile: SourceFile, clientContextClassName: string) {
 
 interface BuildContructorParams {
   clientContextClassName: string;
-  hasCredentials: boolean;
   requiredParams: ParameterDetails[];
 }
 
 function buildConstructor(
   contextClass: ClassDeclaration,
-  {
-    clientContextClassName,
-    hasCredentials,
-    requiredParams
-  }: BuildContructorParams
+  { clientContextClassName, requiredParams }: BuildContructorParams
 ) {
+  const docs = [
+    `Initializes a new instance of the ${clientContextClassName} class.`,
+    ...formatJsDocParam(requiredParams),
+    `@param options The parameter options`
+  ]
+    .filter(d => !!d)
+    .join("\n");
+
   return contextClass.addConstructor({
-    docs: [
-      `Initializes a new instance of the ${clientContextClassName} class.\n
-@param options The parameter options`
-    ],
+    docs: [docs],
     parameters: [
-      ...getCredentialsParameter(hasCredentials),
       ...requiredParams.map(p => ({
         name: p.name,
         type: p.typeDetails.typeName
@@ -212,16 +194,6 @@ function buildConstructor(
   });
 }
 
-function getOptionalParameters(parameters: ParameterDetails[]) {
-  /**
-   * Getting parameters that are not marked as required, and the ones that are not marked as required
-   * but are constants or have a defaultValue
-   */
-  return parameters.filter(
-    p => !p.required || p.defaultValue || p.schemaType === SchemaType.Constant
-  );
-}
-
 function getRequiredParameters(parameters: ParameterDetails[]) {
   /**
    * Getting parameters that are marked as required, and also don't have a defaultValue.
@@ -234,17 +206,18 @@ function getRequiredParameters(parameters: ParameterDetails[]) {
 
 function getBaseUriStatement(baseUrl: BaseUrlDetails) {
   const uri = baseUrl.baseUrl;
-  return `this.baseUri = options.baseUri || this.baseUri ${
-    uri ? ` || "${uri}"` : ""
-  };`;
+  return `this.baseUri = options.baseUri ${uri ? ` || "${uri}"` : ""};`;
 }
 
 function getConstantClientParamAssignments(
   clientParameters: ParameterDetails[]
 ) {
   return clientParameters
     .filter(p => !!p.defaultValue || p.schemaType === SchemaType.Constant)
-    .map(({ name, defaultValue }) => `this.${name} = ${defaultValue}`);
+    .map(
+      ({ name, defaultValue }) =>
+        `this.${name} = options.${name} ||  ${defaultValue}`
+    );
 }
 
 function getRequiredParamChecks(requiredParameters: ParameterDetails[]) {
@@ -255,18 +228,6 @@ function getRequiredParamChecks(requiredParameters: ParameterDetails[]) {
   );
 }
 
-function getOptionalParameterAssignments(
-  optionalParameters: ParameterDetails[]
-) {
-  return optionalParameters.map(
-    ({
-      name
-    }) => `if(options.${name} !== null && options.${name} !== undefined) {
-      this.${name} = options.${name};
-  }`
-  );
-}
-
 function getRequiredParamAssignments(requiredParameters: ParameterDetails[]) {
   return requiredParameters.map(({ name }) => `this.${name} = ${name};`);
 }
diff --git a/src/generators/clientFileGenerator.ts b/src/generators/clientFileGenerator.ts
@@ -15,10 +15,9 @@ import {
   normalizeName,
   NameType
 } from "../utils/nameUtils";
-import { ParameterDetails } from "../models/parameterDetails";
 import { ImplementationLocation, SchemaType } from "@azure-tools/codemodel";
-import { getCredentialsParameter } from "./utils/parameterUtils";
 import { OperationGroupDetails } from "../models/operationDetails";
+import { formatJsDocParam } from "./utils/parameterUtils";
 
 type OperationDeclarationDetails = { name: string; typeName: string };
 
@@ -78,7 +77,7 @@ export function generateClient(clientDetails: ClientDetails, project: Project) {
     extends: clientContextClassName
   });
 
-  writeConstructor(clientDetails, clientClass, hasCredentials);
+  writeConstructor(clientDetails, clientClass);
   writeClientOperations(clientFile, clientClass, clientDetails);
 
   clientFile.addExportDeclaration({
@@ -100,8 +99,7 @@ export function generateClient(clientDetails: ClientDetails, project: Project) {
 
 function writeConstructor(
   clientDetails: ClientDetails,
-  classDeclaration: ClassDeclaration,
-  hasCredentials: boolean
+  classDeclaration: ClassDeclaration
 ) {
   const requiredParams = clientDetails.parameters.filter(
     param =>
@@ -111,13 +109,17 @@ function writeConstructor(
       param.schemaType !== SchemaType.Constant
   );
 
+  const docs = [
+    `Initializes a new instance of the ${clientDetails.className} class.`,
+    ...formatJsDocParam(requiredParams),
+    `@param options The parameter options`
+  ]
+    .filter(d => !!d)
+    .join("\n");
+
   const clientConstructor = classDeclaration.addConstructor({
-    docs: [
-      `Initializes a new instance of the ${clientDetails.className} class.
-@param options The parameter options`
-    ],
+    docs: [docs],
     parameters: [
-      ...getCredentialsParameter(hasCredentials),
       ...requiredParams.map(p => ({
         name: p.name,
         hasQuestionToken: !p.required,
@@ -132,7 +134,7 @@ function writeConstructor(
   });
 
   clientConstructor.addStatements([
-    `super(${getSuperParams(requiredParams, hasCredentials)});`
+    `super(${[...requiredParams.map(p => p.name), "options"].join()});`
   ]);
 
   const operationDeclarationDetails: OperationDeclarationDetails[] = getOperationGroupsDeclarationDetails(
@@ -190,19 +192,3 @@ function writeClientOperations(
     })
   );
 }
-
-function getSuperParams(
-  requiredParams: ParameterDetails[],
-  hasCredentials: boolean
-) {
-  let allParams = ["options"];
-  requiredParams.forEach(p => {
-    allParams = [p.name, ...allParams];
-  });
-
-  if (hasCredentials) {
-    allParams.unshift("credentials");
-  }
-
-  return allParams.join();
-}
diff --git a/src/generators/operationGenerator.ts b/src/generators/operationGenerator.ts
@@ -22,8 +22,12 @@ import {
 } from "../models/operationDetails";
 import { isString } from "util";
 import { ParameterDetails } from "../models/parameterDetails";
-import { filterOperationParameters } from "./utils/parameterUtils";
+import {
+  filterOperationParameters,
+  formatJsDocParam
+} from "./utils/parameterUtils";
 import { PropertyKind } from "../models/modelDetails";
+import { wrapString } from "./utils/stringUtils";
 
 /**
  * Function that writes the code for all the operations.
@@ -308,15 +312,11 @@ function generateOperationJSDoc(
   description: string = ""
 ): string {
   const paramJSDoc =
-    !params || !params.length
-      ? ""
-      : params
-          .map(param => {
-            return `@param ${param.name} ${param.description}`;
-          })
-          .join("\n");
-
-  return `${description ? description + "\n" : description}${paramJSDoc}`;
+    !params || !params.length ? "" : formatJsDocParam(params).join("\n");
+
+  return `${
+    description ? wrapString(description) + "\n" : description
+  }${paramJSDoc}`;
 }
 
 /**

diff --git a/src/generators/parametersGenerator.ts b/src/generators/parametersGenerator.ts
@@ -29,14 +29,16 @@ export function generateParameters(
     moduleSpecifier: "../models/mappers"
   });
 
-  clientDetails.parameters.forEach(param => {
-    parametersFile.addVariableStatement({
-      isExported: true,
-      declarations: [buildParameterInitializer(param)],
-      declarationKind: VariableDeclarationKind.Const,
-      leadingTrivia: writer => writer.blankLine()
+  clientDetails.parameters
+    .filter(p => !p.isSynthetic)
+    .forEach(param => {
+      parametersFile.addVariableStatement({
+        isExported: true,
+        declarations: [buildParameterInitializer(param)],
+        declarationKind: VariableDeclarationKind.Const,
+        leadingTrivia: writer => writer.blankLine()
+      });
     });
-  });
 }
 
 function buildParameterInitializer(parameter: ParameterDetails) {

diff --git a/src/generators/utils/parameterUtils.ts b/src/generators/utils/parameterUtils.ts
@@ -4,7 +4,7 @@
 import { ParameterDetails } from "../../models/parameterDetails";
 import { OperationDetails } from "../../models/operationDetails";
 import { ImplementationLocation, SchemaType } from "@azure-tools/codemodel";
-import { StructureKind, ParameterDeclarationStructure } from "ts-morph";
+import { wrapString, IndentationType } from "./stringUtils";
 
 interface ParameterFilterOptions {
   includeOptional?: boolean;
@@ -59,24 +59,11 @@ export function filterOperationParameters(
   );
 }
 
-export function getCredentialsParameter(
-  hasCredentials: boolean
-): ParameterDeclarationStructure[] {
-  return hasCredentials
-    ? [
-        {
-          name: "credentials",
-          type: `coreHttp.TokenCredential | coreHttp.ServiceClientCredentials`,
-          kind: StructureKind.Parameter
-        }
-      ]
-    : [];
-}
-
-export function getCredentialsCheck(hasCredentials: boolean): string {
-  return hasCredentials
-    ? `if (credentials == undefined) {
-    throw new Error("'credentials' cannot be null.");
-  }`
-    : ``;
+export function formatJsDocParam(parameters: Partial<ParameterDetails>[]) {
+  return parameters.map(p =>
+    wrapString(`@param ${p.name} ${p.description}`, {
+      indentationType: IndentationType.JSDocParam,
+      paramNameLength: p.name?.length
+    })
+  );
 }