feat: add support for comments on union fields in generateOneofProper…

…ty (#1136) - Added functionality to `generateOneofProperty` to include comments for each union field, enhancing documentation support for union fields in TypeScript output. - Previously, comments for individual fields within a `oneof` type were omitted. This update gathers comments from each field and appends them to the generated type for clarity. Issue: [#1122](#1122)
stephenh · Nov 16, 2024 · c933c9c · c933c9c
1 parent 6100390
commit c933c9c
Show file tree

Hide file tree

Showing 8 changed files with 173 additions and 75 deletions.
diff --git a/integration/oneof-unions-snake/google/protobuf/struct.ts b/integration/oneof-unions-snake/google/protobuf/struct.ts
@@ -69,13 +69,26 @@ export interface Struct_FieldsEntry {
  * The JSON representation for `Value` is JSON value.
  */
 export interface Value {
+  /** The kind of value. */
   kind?:
-    | { $case: "null_value"; null_value: NullValue }
-    | { $case: "number_value"; number_value: number }
-    | { $case: "string_value"; string_value: string }
-    | { $case: "bool_value"; bool_value: boolean }
-    | { $case: "struct_value"; struct_value: { [key: string]: any } | undefined }
-    | { $case: "list_value"; list_value: Array<any> | undefined }
+    | //
+    /** Represents a null value. */
+    { $case: "null_value"; null_value: NullValue }
+    | //
+    /** Represents a double value. */
+    { $case: "number_value"; number_value: number }
+    | //
+    /** Represents a string value. */
+    { $case: "string_value"; string_value: string }
+    | //
+    /** Represents a boolean value. */
+    { $case: "bool_value"; bool_value: boolean }
+    | //
+    /** Represents a structured value. */
+    { $case: "struct_value"; struct_value: { [key: string]: any } | undefined }
+    | //
+    /** Represents a repeated `Value`. */
+    { $case: "list_value"; list_value: Array<any> | undefined }
     | undefined;
 }
 

diff --git a/integration/oneof-unions-value/google/protobuf/struct.ts b/integration/oneof-unions-value/google/protobuf/struct.ts
@@ -69,13 +69,26 @@ export interface Struct_FieldsEntry {
  * The JSON representation for `Value` is JSON value.
  */
 export interface Value {
+  /** The kind of value. */
   kind?:
-    | { $case: "nullValue"; value: NullValue }
-    | { $case: "numberValue"; value: number }
-    | { $case: "stringValue"; value: string }
-    | { $case: "boolValue"; value: boolean }
-    | { $case: "structValue"; value: { [key: string]: any } | undefined }
-    | { $case: "listValue"; value: Array<any> | undefined }
+    | //
+    /** Represents a null value. */
+    { $case: "nullValue"; value: NullValue }
+    | //
+    /** Represents a double value. */
+    { $case: "numberValue"; value: number }
+    | //
+    /** Represents a string value. */
+    { $case: "stringValue"; value: string }
+    | //
+    /** Represents a boolean value. */
+    { $case: "boolValue"; value: boolean }
+    | //
+    /** Represents a structured value. */
+    { $case: "structValue"; value: { [key: string]: any } | undefined }
+    | //
+    /** Represents a repeated `Value`. */
+    { $case: "listValue"; value: Array<any> | undefined }
     | undefined;
 }
 

diff --git a/integration/oneof-unions-value/oneof.ts b/integration/oneof-unions-value/oneof.ts
@@ -9,19 +9,45 @@ export const protobufPackage = "oneof";
 
 export interface PleaseChoose {
   name: string;
+  /**
+   * Please to be choosing one of the fields within this oneof clause.
+   * This text exists to ensure we transpose comments correctly.
+   */
   choice?:
-    | { $case: "aNumber"; value: number }
-    | { $case: "aString"; value: string }
-    | { $case: "aMessage"; value: PleaseChoose_Submessage }
-    | { $case: "aBool"; value: boolean }
-    | { $case: "bunchaBytes"; value: Uint8Array }
-    | { $case: "anEnum"; value: PleaseChoose_StateEnum }
+    | //
+    /**
+     * Use this if you want a number. Numbers are great. Who doesn't
+     * like them?
+     */
+    { $case: "aNumber"; value: number }
+    | //
+    /**
+     * Use this if you want a string. Strings are also nice. Not as
+     * nice as numbers, but what are you going to do...
+     */
+    { $case: "aString"; value: string }
+    | //
+    { $case: "aMessage"; value: PleaseChoose_Submessage }
+    | //
+    /**
+     * We also added a bool option! This was added after the 'age'
+     * field, so it has a higher number.
+     */
+    { $case: "aBool"; value: boolean }
+    | //
+    { $case: "bunchaBytes"; value: Uint8Array }
+    | //
+    { $case: "anEnum"; value: PleaseChoose_StateEnum }
     | undefined;
   age: number;
-  eitherOr?: { $case: "either"; value: string } | { $case: "or"; value: string } | {
-    $case: "thirdOption";
-    value: string;
-  } | undefined;
+  eitherOr?:
+    | //
+    { $case: "either"; value: string }
+    | //
+    { $case: "or"; value: string }
+    | //
+    { $case: "thirdOption"; value: string }
+    | undefined;
   signature: Uint8Array;
   value: any | undefined;
 }

diff --git a/integration/oneof-unions/google/protobuf/struct.ts b/integration/oneof-unions/google/protobuf/struct.ts
@@ -69,13 +69,26 @@ export interface Struct_FieldsEntry {
  * The JSON representation for `Value` is JSON value.
  */
 export interface Value {
+  /** The kind of value. */
   kind?:
-    | { $case: "nullValue"; nullValue: NullValue }
-    | { $case: "numberValue"; numberValue: number }
-    | { $case: "stringValue"; stringValue: string }
-    | { $case: "boolValue"; boolValue: boolean }
-    | { $case: "structValue"; structValue: { [key: string]: any } | undefined }
-    | { $case: "listValue"; listValue: Array<any> | undefined }
+    | //
+    /** Represents a null value. */
+    { $case: "nullValue"; nullValue: NullValue }
+    | //
+    /** Represents a double value. */
+    { $case: "numberValue"; numberValue: number }
+    | //
+    /** Represents a string value. */
+    { $case: "stringValue"; stringValue: string }
+    | //
+    /** Represents a boolean value. */
+    { $case: "boolValue"; boolValue: boolean }
+    | //
+    /** Represents a structured value. */
+    { $case: "structValue"; structValue: { [key: string]: any } | undefined }
+    | //
+    /** Represents a repeated `Value`. */
+    { $case: "listValue"; listValue: Array<any> | undefined }
     | undefined;
 }
 

diff --git a/integration/oneof-unions/oneof.ts b/integration/oneof-unions/oneof.ts
@@ -9,19 +9,45 @@ export const protobufPackage = "oneof";
 
 export interface PleaseChoose {
   name: string;
+  /**
+   * Please to be choosing one of the fields within this oneof clause.
+   * This text exists to ensure we transpose comments correctly.
+   */
   choice?:
-    | { $case: "aNumber"; aNumber: number }
-    | { $case: "aString"; aString: string }
-    | { $case: "aMessage"; aMessage: PleaseChoose_Submessage }
-    | { $case: "aBool"; aBool: boolean }
-    | { $case: "bunchaBytes"; bunchaBytes: Uint8Array }
-    | { $case: "anEnum"; anEnum: PleaseChoose_StateEnum }
+    | //
+    /**
+     * Use this if you want a number. Numbers are great. Who doesn't
+     * like them?
+     */
+    { $case: "aNumber"; aNumber: number }
+    | //
+    /**
+     * Use this if you want a string. Strings are also nice. Not as
+     * nice as numbers, but what are you going to do...
+     */
+    { $case: "aString"; aString: string }
+    | //
+    { $case: "aMessage"; aMessage: PleaseChoose_Submessage }
+    | //
+    /**
+     * We also added a bool option! This was added after the 'age'
+     * field, so it has a higher number.
+     */
+    { $case: "aBool"; aBool: boolean }
+    | //
+    { $case: "bunchaBytes"; bunchaBytes: Uint8Array }
+    | //
+    { $case: "anEnum"; anEnum: PleaseChoose_StateEnum }
     | undefined;
   age: number;
-  eitherOr?: { $case: "either"; either: string } | { $case: "or"; or: string } | {
-    $case: "thirdOption";
-    thirdOption: string;
-  } | undefined;
+  eitherOr?:
+    | //
+    { $case: "either"; either: string }
+    | //
+    { $case: "or"; or: string }
+    | //
+    { $case: "thirdOption"; thirdOption: string }
+    | undefined;
   signature: Uint8Array;
   value: any | undefined;
 }

diff --git a/integration/use-readonly-types/google/protobuf/struct.ts b/integration/use-readonly-types/google/protobuf/struct.ts
@@ -69,13 +69,26 @@ export interface Struct_FieldsEntry {
  * The JSON representation for `Value` is JSON value.
  */
 export interface Value {
+  /** The kind of value. */
   readonly kind?:
-    | { readonly $case: "nullValue"; readonly nullValue: NullValue }
-    | { readonly $case: "numberValue"; readonly numberValue: number }
-    | { readonly $case: "stringValue"; readonly stringValue: string }
-    | { readonly $case: "boolValue"; readonly boolValue: boolean }
-    | { readonly $case: "structValue"; readonly structValue: { readonly [key: string]: any } | undefined }
-    | { readonly $case: "listValue"; readonly listValue: ReadonlyArray<any> | undefined }
+    | //
+    /** Represents a null value. */
+    { readonly $case: "nullValue"; readonly nullValue: NullValue }
+    | //
+    /** Represents a double value. */
+    { readonly $case: "numberValue"; readonly numberValue: number }
+    | //
+    /** Represents a string value. */
+    { readonly $case: "stringValue"; readonly stringValue: string }
+    | //
+    /** Represents a boolean value. */
+    { readonly $case: "boolValue"; readonly boolValue: boolean }
+    | //
+    /** Represents a structured value. */
+    { readonly $case: "structValue"; readonly structValue: { readonly [key: string]: any } | undefined }
+    | //
+    /** Represents a repeated `Value`. */
+    { readonly $case: "listValue"; readonly listValue: ReadonlyArray<any> | undefined }
     | undefined;
 }
 

diff --git a/integration/use-readonly-types/use-readonly-types.ts b/integration/use-readonly-types/use-readonly-types.ts
@@ -19,10 +19,12 @@ export interface Entity {
   readonly fieldMask: readonly string[] | undefined;
   readonly listValue: ReadonlyArray<any> | undefined;
   readonly structValue: { readonly [key: string]: any } | undefined;
-  readonly oneOfValue?: { readonly $case: "theStringValue"; readonly theStringValue: string } | {
-    readonly $case: "theIntValue";
-    readonly theIntValue: number;
-  } | undefined;
+  readonly oneOfValue?:
+    | //
+    { readonly $case: "theStringValue"; readonly theStringValue: string }
+    | //
+    { readonly $case: "theIntValue"; readonly theIntValue: number }
+    | undefined;
 }
 
 export interface SubEntity {

diff --git a/src/main.ts b/src/main.ts
@@ -1174,41 +1174,33 @@ function generateOneofProperty(
   sourceInfo: SourceInfo,
 ): Code {
   const { options } = ctx;
-  const fields = messageDesc.field.filter((field) => isWithinOneOf(field) && field.oneofIndex === oneofIndex);
+  const fields = messageDesc.field
+    .map((field, index) => ({ index, field }))
+    .filter((item) => isWithinOneOf(item.field) && item.field.oneofIndex === oneofIndex);
+
   const mbReadonly = maybeReadonly(options);
+  const info = sourceInfo.lookup(Fields.message.oneof_decl, oneofIndex);
+  let outerComments: Code[] = [];
+  maybeAddComment(options, info, outerComments);
+
   const unionType = joinCode(
-    fields.map((f) => {
-      let fieldName = maybeSnakeToCamel(f.name, options);
-      let typeName = toTypeName(ctx, messageDesc, f);
+    fields.flatMap((f) => {
+      const fieldInfo = sourceInfo.lookup(Fields.message.field, f.index);
+      let fieldName = maybeSnakeToCamel(f.field.name, options);
+      let typeName = toTypeName(ctx, messageDesc, f.field);
       let valueName = oneofValueName(fieldName, options);
-      return code`{ ${mbReadonly}$case: '${fieldName}', ${mbReadonly}${valueName}: ${typeName} }`;
+      let fieldComments: Code[] = [];
+      maybeAddComment(options, fieldInfo, fieldComments);
+
+      const combinedComments = fieldComments.join("\n");
+      return code`| // \n ${combinedComments} { ${mbReadonly}$case: '${fieldName}', ${mbReadonly}${valueName}: ${typeName} }`;
     }),
-    { on: " | " },
   );
 
   const name = maybeSnakeToCamel(messageDesc.oneofDecl[oneofIndex].name, options);
-  return code`${mbReadonly}${name}?: ${unionType} | ${nullOrUndefined(options)},`;
-
-  /*
-  // Ideally we'd put the comments for each oneof field next to the anonymous
-  // type we've created in the type union above, but ts-poet currently lacks
-  // that ability. For now just concatenate all comments into one big one.
-  let comments: Array<string> = [];
-  const info = sourceInfo.lookup(Fields.message.oneof_decl, oneofIndex);
-  maybeAddComment(options, info, (text) => comments.push(text));
-  messageDesc.field.forEach((field, index) => {
-    if (!isWithinOneOf(field) || field.oneofIndex !== oneofIndex) {
-      return;
-    }
-    const info = sourceInfo.lookup(Fields.message.field, index);
-    const name = maybeSnakeToCamel(field.name, options);
-    maybeAddComment(options, info, (text) => comments.push(name + '\n' + text));
+  return joinCode([...outerComments, code`${mbReadonly}${name}?:`, unionType, code`| ${nullOrUndefined(options)},`], {
+    on: "\n",
   });
-  if (comments.length) {
-    prop = prop.addJavadoc(comments.join('\n'));
-  }
-  return prop;
-  */
 }
 
 // Create a function that constructs 'base' instance with default values for decode to use as a prototype