chore: respond to comments

Author: sofisl

packages/gapic-node-processing/src/combine-libraries.ts

@@ -17,6 +17,14 @@ import {Dirent} from 'fs';
 const fs = require('fs/promises'); // For async file system operations
 const path = require('path');
 
+export interface FilePaths {
+  filePath: string
+}
+
+export interface FilePathsAndContents {
+  filePath: string,
+  content: string
+}
 /**
  * Recursively removes a regex pattern from a specified property in an array of objects.
  *
@@ -63,7 +71,7 @@ export function removeRegexFromNestedProperty(
  * @returns A promise that resolves when all file contents have been read and added to the array.
  */
 async function readFilesContent(
-  filePaths: {filePath: string}[],
+  filePaths: FilePaths[],
 ): Promise<void> {
   const promises = filePaths.map(async item => {
     try {
@@ -128,7 +136,7 @@ export async function traverseDirectory(
  */
 export async function generateFinalDirectoryPath(
   currentPath: string,
-): Promise<Array<{filePath: string; content: string}>> {
+): Promise<Array<FilePathsAndContents>> {
   // Get a full list of all the file paths in all the libraries
   let fullPaths: {filePath: string}[] = [];
   let fullPathsAndContents: {filePath: string; content: string}[] = [];
@@ -142,12 +150,12 @@ export async function generateFinalDirectoryPath(
       path.join(currentPath, directory),
     );
     fullPathsAndContents = fullPathsAndContents.concat(
-      fullPaths as {filePath: string; content: string}[],
+      fullPaths as FilePathsAndContents[],
     );
   }
 
   // Now we need to clean out duplicates
-  const uniquePaths = new Set();
+  const uniquePaths = new Set<string>();
   const uniquefullPathAndContent = [];
 
   for (const fullPathAndContent of fullPathsAndContents) {
@@ -174,8 +182,8 @@ export async function ensureDirectoryExists(filePath: string): Promise<void> {
   try {
     await fs.mkdir(dirPath, {recursive: true});
   } catch (error) {
+    // EEXIST means it already exists, which is fine
     if ((error as any).code !== 'EEXIST') {
-      // EEXIST means it already exists, which is fine
       console.error(`Error ensuring directory ${dirPath} exists:`, error);
       throw error;
     }

packages/gapic-node-processing/src/commands/generate-combined-library.ts

@@ -15,12 +15,14 @@
 import yargs = require('yargs');
 import {combineLibraries} from '../combine-libraries';
 import {generateIndexTs} from '../generate-index';
+
 export interface CliArgs {
   'source-path': string;
   'default-version'?: string;
   'destination-path'?: string;
   'is-esm'?: boolean;
 }
+
 /**
  * Command module for bootstrapping a library.
  *

packages/gapic-node-processing/src/commands/generate-readme.ts

@@ -15,7 +15,7 @@
 import yargs = require('yargs');
 import {initialGenerateReadMe, readAndWriteToReadme} from '../generate-readme';
 
-export interface CliArgs {
+export interface CliArgsReadme {
   'source-path': string;
   'initial-generation'?: boolean;
   'destination-path'?: string;
@@ -29,7 +29,7 @@ export interface CliArgs {
 const DEFAULT_REPLACEMENT_STRING_SAMPLES = '[//]: # "samples"';
 const DEFAULT_REPLACEMENT_STRING_RELEASE_LEVEL = '[//]: # "releaseLevel"';
 
-function validateCliArgs(argv: CliArgs) {
+function validateCliArgs(argv: CliArgsReadme) {
   if (
     !argv['initial-generation'] &&
     (!argv['string-to-replace'] || !argv['replacement-string'])
@@ -45,7 +45,7 @@ function validateCliArgs(argv: CliArgs) {
   }
 }
 
-function getReplacementStringSamples(argv: CliArgs) {
+function getReplacementStringSamples(argv: CliArgsReadme) {
   const stringToReplaceForSampleTable = argv['replacement-string-samples'];
   if (argv['initial-generation'] && !stringToReplaceForSampleTable) {
     console.log(
@@ -57,7 +57,7 @@ function getReplacementStringSamples(argv: CliArgs) {
   return stringToReplaceForSampleTable;
 }
 
-function getReplacementStringReleaseLevel(argv: CliArgs) {
+function getReplacementStringReleaseLevel(argv: CliArgsReadme) {
   const stringToReplaceForReleaseLevel =
     argv['replacement-string-release-level'];
   if (argv['initial-generation'] && !stringToReplaceForReleaseLevel) {
@@ -70,7 +70,7 @@ function getReplacementStringReleaseLevel(argv: CliArgs) {
   return stringToReplaceForReleaseLevel;
 }
 
-function generateArgsForInitialReadme(argv: CliArgs, writeDestination: string) {
+function generateArgsForInitialReadme(argv: CliArgsReadme, writeDestination: string) {
   const stringToReplaceForSampleTable = getReplacementStringSamples(argv);
   const stringToReplaceForReleaseLevel = getReplacementStringReleaseLevel(argv);
   return {
@@ -87,7 +87,7 @@ function generateArgsForInitialReadme(argv: CliArgs, writeDestination: string) {
  *
  * This module defines a yargs command to generate a README.md
  */
-export const generateReadme: yargs.CommandModule<{}, CliArgs> = {
+export const generateReadme: yargs.CommandModule<{}, CliArgsReadme> = {
   command: 'generate-readme',
   describe: 'Combines the versions for a given API into a single library',
   builder(yargs) {
@@ -131,15 +131,15 @@ export const generateReadme: yargs.CommandModule<{}, CliArgs> = {
    * Yargs command handler for generating a combined library.
    *
    * @param {CliArgs} argv - The command line arguments
-   * @param {CliArgs} source-path: path in which a pre-combined library lives
-   * @param {CliArgs} destination-path: path where to copy over the library
-   * @param {CliArgs} initial-generation: path in which a pre-combined library lives; defaults to false
-   * @param {CliArgs} release-level: what is the release level of the library (default is preview)
-   * @param {CliArgs} string-to-replace: string to replace in the readme
-   * @param {CliArgs} replacement-string-samples: string to replace with the string-to-replace for the samples table in the readme (only used with initial generation)
-   * @param {CliArgs} replacement-string-release-level: string to replace with the string-to-replace for the releaseLevel in the readme (only used with initial generation)
+   * source-path: path in which a pre-combined library lives
+   * destination-path: path where to copy over the library
+   * initial-generation: path in which a pre-combined library lives; defaults to false
+   * release-level: what is the release level of the library (default is preview)
+   * string-to-replace: string to replace in the readme
+   * replacement-string-samples: string to replace with the string-to-replace for the samples table in the readme (only used with initial generation)
+   * replacement-string-release-level: string to replace with the string-to-replace for the releaseLevel in the readme (only used with initial generation)
    */
-  async handler(argv: CliArgs) {
+  async handler(argv: CliArgsReadme) {
     const destinationPath = argv['destination-path'] || argv['source-path'];
     validateCliArgs(argv);
     if (argv['initial-generation']) {

packages/gapic-node-processing/src/generate-index.ts

@@ -22,6 +22,10 @@ const SRC_PATH = 'src';
 const INDEX_PATH = 'index.ts';
 const TEMPLATE_FILE_NAME = 'index.ts.njk';
 
+export interface VersionsAndClients {
+  version: string
+  clients: string[]
+}
 export const POST_PROCESSING_TEMPLATES_PATH = path.resolve(
   __dirname,
   '../../templates/post-processing-templates',
@@ -80,7 +84,7 @@ export async function extractVersions(currentPath: string) {
  */
 export async function extractClients(currentPath: string) {
   const directories = await extractVersions(currentPath);
-  const clientsAndVersions: {version: string; clients: string[]}[] = [];
+  const clientsAndVersions: VersionsAndClients[] = [];
   for (const directory of directories) {
     const indexFile = path.join(currentPath, SRC_PATH, directory, INDEX_PATH);
     if (await fs.stat(indexFile)) {
@@ -108,6 +112,7 @@ export async function extractClients(currentPath: string) {
  *
  * @param {string} currentLibrary - The path to the library's root directory.
  * @param {string} [defaultVersion] - An optional version string to explicitly set as the default.
+ * @param {string} [isEsm] - An optional flag whether the library should be generated with ESM syntax
  */
 export async function generateIndexTs(
   currentLibrary: string,
@@ -133,7 +138,7 @@ export async function generateIndexTs(
   );
 
   // Render index.ts
-  const variables = {versions, defaultClientAndVersions, isEsm: isEsm || false};
+  const variables = {versions, defaultClientAndVersions, isEsm: isEsm ?? false};
 
   // Create a new Nunjucks environment configured to load from the templateDirectory
   // This is necessary due to occurring in a Bazel environment or locally
@@ -151,6 +156,7 @@ export async function generateIndexTs(
   await fs.writeFile(outputPath, compiledTemplate);
   console.log(`Successfully wrote: ${outputPath}`);
 }
+
 interface VersionSpec {
   version: string; // Stores the "best" version found so far
   major: number;
@@ -164,7 +170,7 @@ interface VersionSpec {
 // 3. Within same pre-release type, higher qualifier (e.g., beta2 > beta1)
 // 4. If everything else is equal, the current one is just as good
 
-function isANewAHighestVersion(
+function isNewAHighestVersion(
   currentVersionSpec: VersionSpec,
   newVersionSpec: VersionSpec,
 ): boolean {
@@ -238,7 +244,7 @@ export function getHighestVersionWithPrecedence(versions: string[]) {
         preReleaseQualifier: preReleaseQualifier,
       };
 
-      if (isANewAHighestVersion(currentVersionSpec, newVersionSpec)) {
+      if (isNewAHighestVersion(currentVersionSpec, newVersionSpec)) {
         currentVersionSpec = newVersionSpec;
       }
     }

packages/gapic-node-processing/src/generate-readme.ts

@@ -12,9 +12,10 @@
 // See the License for the specific language governing permissions and
 // limitations under the License.
 
-import {ensureDirectoryExists, removeRegexFromNestedProperty, traverseDirectory} from './combine-libraries';
+import {ensureDirectoryExists, traverseDirectory, FilePaths} from './combine-libraries';
 import * as nj from 'nunjucks';
 import {POST_PROCESSING_TEMPLATES_PATH} from './generate-index';
+import { CliArgsReadme } from './commands/generate-readme';
 
 const fs = require('fs/promises'); // For async file system operations
 const path = require('path');
@@ -31,6 +32,12 @@ export const RELEASE_LEVEL_PREVIEW = `This library is considered to be in **prev
 work-in-progress and under active development. Any release is subject to
 backwards-incompatible changes at any time.`;
 
+interface SampleMetadata {
+  filePath: string,
+  content: string,
+  title: string
+}
+
 const DEFAULT_RELEASE_LEVEL = 'preview';
 
 /**
@@ -41,33 +48,36 @@ const DEFAULT_RELEASE_LEVEL = 'preview';
  * the file path, content, and title for each sample.
  *
  * @param {string} currentLibrary - The path to the library's root directory.
- * @returns {Promise<{filePath: string; content: string; title: string}[]>} A promise that resolves to an array of sample metadata objects.
+ * @returns {Promise<SampleMetadata[]>} A promise that resolves to an array of sample metadata objects.
  */
 export async function getSamplesMetadata(
   currentLibrary: string,
-): Promise<{filePath: string; content: string; title: string}[]> {
-  // Let's remove the main library from the samples paths
+): Promise<SampleMetadata[]> {
+  // Let's separate out the absolute path so that we
+  // can later remove it from the filePath (so that it
+  // is relative to the working directory). 
+  // For example for CURRENT LIBRARY: google-cloud-node/packages/gapic-node-processing/test/fixtures/combined-library/google-cloud-speech
+  // we would produce stringToRemove: /Users/sofialeon/gcp/google-cloud-node/packages/gapic-node-processing/test/fixtures/combined-library
+  // which we then use to remove from the absolute filePath (so
+  // /google-cloud-node/packages/gapic-node-processing/test/fixtures/combined-library/google-cloud-speech/samples/generated/v1/adaptation.create_custom_class.js
+  // becomes just google-cloud-speech/samples/generated/v1/adaptation.create_custom_class.js (the relative path to the new directory)
   const stringToRemove = currentLibrary
     .split('/')
     .slice(0, currentLibrary.split('/').length - 1)
     .join('/');
-  let samples = await traverseDirectory(
+
+  const samples = await traverseDirectory(
     path.join(currentLibrary, SAMPLES_PATH),
     [], 
   );
-  removeRegexFromNestedProperty(samples, 'filePath', stringToRemove)
   samples.map(sample => {
-    // Here, the 'getSampleName' refers to the exported function
+    sample.filePath = sample.filePath.replace(stringToRemove, '').replace('-nodejs', '');
     Object.assign(sample, {title: getSampleName(sample)});
   });
-  // Remove 'nodejs' from the library title
-  removeRegexFromNestedProperty(samples, 'filePath', '-nodejs')
-  // Since we later assign the title property, it should exist
-  return samples as unknown as {
-    filePath: string;
-    content: string;
-    title: string;
-  }[];
+  console.log(samples)
+  // Since we later assign the title property, we can coerce it into
+  // this type
+  return samples as unknown as SampleMetadata[];
 }
 
 /**
@@ -77,7 +87,7 @@ export async function getSamplesMetadata(
  * stripping the file extension, API version, and replacing underscores with spaces.
  * If the path is not in the expected format, it logs an error and returns the original path.
  *
- * @param {{filePath: string; content: string}} sample - The sample object containing the file path.
+ * @param {FilePaths} sample - The sample object containing the file path.
  * @returns {string} The formatted name of the sample.
  */
 export function getSampleName(sample: {
@@ -103,16 +113,14 @@ export function getSampleName(sample: {
   return sampleName;
 }
 
-// We have two readme generations because the initial generation step
-// will be a little different than what's available during post-processing
 /**
  * Generates the initial README file for a library by compiling templates and replacing placeholders.
  *
  * This function uses Nunjucks to render a sample table based on the found samples
  * and injects it along with the appropriate release level message into the README.md
  * template.
  *
- * @param {{currentLibrary: string; stringToReplaceForSampleTable: string; stringToReplaceForReleaseLevel: string; writeLibrary: string; releaseLevel?: string;}} args - An object containing the arguments for the function.
+ * @param {CliArgsReadme} args - An object containing the arguments for the function.
  * @param {string} args.currentLibrary - The path to the library's root directory.
  * @param {string} args.stringToReplaceForSampleTable - The placeholder string to be replaced with the samples table.
  * @param {string} args.stringToReplaceForReleaseLevel - The placeholder string to be replaced with the release level message.

packages/gapic-node-processing/test/generate-readme.test.ts

@@ -69,7 +69,7 @@ describe('generate readme.ts', () => {
     assert.ok(samples.map(x => x.title).includes('undelete recognizer'));
 
     // Assert content exists
-    assert.ok(samples.map(x => x.content));
+    assert.ok(samples.map(x => x.content).length);
   });
 
   it('should NOT throw if the sample name is not in the correct format; just return the name', async () => {