Fix array of enums #335

joeldodge79 · 2020-09-29T15:20:50Z

A property that is an array of enum values was being generated as only
that enum. This change corrects that.

fixes #334

A property that is an array of enum values was being generated as only that enum. This change corrects that.

joeldodge79

some comments/questions

joeldodge79 · 2020-09-29T15:21:26Z

csharp/sdk/3.1/models.cs

@@ -1735,20 +1735,15 @@ public class Integration : SdkModel
  /// <summary>Array of params for the integration.</summary>
  public IntegrationParam[]? @params { get; set; } = null;
  /// <summary>A list of data formats the integration supports. If unspecified, the default is all data formats. Valid values are: "txt", "csv", "inline_json", "json", "json_label", "json_detail", "json_detail_lite_stream", "xlsx", "html", "wysiwyg_pdf", "assembled_pdf", "wysiwyg_png", "csv_zip". (read-only)</summary>
-  [JsonConverter(typeof(StringEnumConverter))]
-  public SupportedFormats? supported_formats { get; set; }
+  public SupportedFormats[]? supported_formats { get; set; } = null;


is this correct syntax?

joeldodge79 · 2020-09-29T15:24:06Z

packages/sdk-codegen/src/csharp.gen.spec.ts

@@ -146,26 +146,9 @@ describe('c# generator', () => {
          .user_attribute_filter_types.type
      const actual = gen.declareType('', type)
      const expected = `/// An array of user attribute types that are allowed to be used in filters on this field. Valid values are: "advanced_filter_string", "advanced_filter_number", "advanced_filter_datetime", "string", "number", "datetime", "relative_url", "yesno", "zipcode".
-public enum UserAttributeFilterTypes
+public class UserAttributeFilterTypes[] : SdkModel


I have no idea if this is correct - it's just what the actual output is right now

This is not correct

joeldodge79 · 2020-09-29T15:24:36Z

packages/sdk-codegen/src/go.gen.ts

-          .toUpperCase()
-          .replace('-', '')
-          .replace('_', '')
+        group.toUpperCase().replace('-', '').replace('_', '')


prettier did this

joeldodge79 · 2020-09-29T15:26:35Z

packages/sdk-codegen/src/sdkModels.spec.ts

@@ -450,7 +458,7 @@ describe('sdkModels', () => {
    })

    describe('enum naming', () => {
-      const rf1: OAS.SchemaObject = {
+      const rf: OAS.SchemaObject = {


renumbered so that expect(actual1.name).toEqual('ResultFormat1') below is easier on the brain

joeldodge79 · 2020-09-29T15:28:00Z

packages/sdk-codegen/src/sdkModels.ts

        if (propType.instanceOf('EnumType')) {
          api.registerEnum(propType, propName)
        }


Is this necessary? resolveType does a registerEnum any time it finds one.

Pretty sure if you take that line out enum resolution will break

joeldodge79 · 2020-09-29T15:32:12Z

packages/sdk-codegen/src/sdkModels.ts

+  constructor(
+    public elementType: IType,
+    schema: OAS.SchemaObject,
+    types: TypeList,
+    typeName?: string,
+    methodName?: string
+  ) {


I lost some time because I didn't immediately see where the enum name was being switched from result_format to ResultFormat. I don't think this solution is totally ideal but I also don't think that setting the enum name in a function called registerEnum is right either. If knowledge of how to set the name should not be contained inside the EnumType class (because we need to know types, typeName, and methodName which are external to the object) then perhaps a separate method named makeEnum() that instantiates the object and then sets its name. But I still think registerEnum() isn't the right place for it.

I'm happy to discuss the enum naming logic. I put it in the place where the enum was added to the model because that's where it was needed. I don't know how registerEnum() would be different than makeEnum(), but again, let's talk about it.

maybe it's just how my brain works but I was reading the new EnumType(...) as "here's the enum in all it's entirety" and my brain glanced over the registerEnum call because I didn't think that would have a side effect on the enum object itself. So it took me a little while to figure out that it was changing the name. I didn't think I'd need to read the contents of the registerEnum method to learn about how properties of the enum instance itself were set. I'd expect that to be found more in either the constructor or a method with a more aggressively mutating sounding name :-)

joeldodge79 · 2020-09-29T15:46:10Z

packages/sdk-codegen/src/sdkModels.ts

+    let name = titleCase(this.name || typeName || 'Enum')
+    if (name in types) {
+      // Enum values don't match existing enum of this name. Pick a new name
+      const baseName = methodName ? titleCase(`${methodName}_${name}`) : name


I think we need to also scope to parentType name to try as hard as possible to avoid the ++i appending. maybe something like

Suggested change

const baseName = methodName ? titleCase(`${methodName}_${name}`) : name

const baseName = methodName ? titleCase(`${methodName}_${name}`) : name

baseName = (baseName in types) ? titleCase(`${parentTypeName}_${baseName}) : baseName

and we'd have to optionally pass parentTypeName through the constructor into this method.

I'll need time to get my head back into this space to have significant feedback :)

joeldodge79 · 2020-09-29T15:47:13Z

packages/sdk-codegen/src/typescript.gen.spec.ts

@@ -842,6 +842,29 @@ export enum ResultFormat {
 export enum Align {
  left = 'left',
  right = 'right'
+}`)
+      })
+      it('array of enums', () => {


I added a test for this in typescript (as well as augmenting a test in python) but I haven't touched the other language gen tests...

spec/Looker.3.0.oas.json

jkaster

The C# enum array syntax is not correct. Need to try building this in Rider or VS Code. Would be nice to have a Look# smoke test, of course.

jkaster · 2020-09-29T21:29:22Z

csharp/sdk/3.1/models.cs

@@ -1735,20 +1735,15 @@ public class Integration : SdkModel
  /// <summary>Array of params for the integration.</summary>
  public IntegrationParam[]? @params { get; set; } = null;
  /// <summary>A list of data formats the integration supports. If unspecified, the default is all data formats. Valid values are: "txt", "csv", "inline_json", "json", "json_label", "json_detail", "json_detail_lite_stream", "xlsx", "html", "wysiwyg_pdf", "assembled_pdf", "wysiwyg_png", "csv_zip". (read-only)</summary>
-  [JsonConverter(typeof(StringEnumConverter))]
-  public SupportedFormats? supported_formats { get; set; }
+  public SupportedFormats[]? supported_formats { get; set; } = null;


jkaster · 2020-09-29T21:32:00Z

packages/sdk-codegen/src/csharp.gen.spec.ts

@@ -146,26 +146,9 @@ describe('c# generator', () => {
          .user_attribute_filter_types.type
      const actual = gen.declareType('', type)
      const expected = `/// An array of user attribute types that are allowed to be used in filters on this field. Valid values are: "advanced_filter_string", "advanced_filter_number", "advanced_filter_datetime", "string", "number", "datetime", "relative_url", "yesno", "zipcode".
-public enum UserAttributeFilterTypes
+public class UserAttributeFilterTypes[] : SdkModel


This is not correct

jkaster · 2020-09-29T21:32:21Z

packages/sdk-codegen/src/go.gen.ts

-          .toUpperCase()
-          .replace('-', '')
-          .replace('_', '')
+        group.toUpperCase().replace('-', '').replace('_', '')


jkaster · 2020-09-29T21:34:28Z

packages/sdk-codegen/src/sdkModels.ts

        if (propType.instanceOf('EnumType')) {
          api.registerEnum(propType, propName)
        }


Pretty sure if you take that line out enum resolution will break

jkaster

LGTM. Thanks for this fix and the thorough review of enum logic.

joeldodge79 · 2020-11-18T15:50:32Z

go/sdk/v3/models.go

+  Description                       *string                              `json:"description,omitempty"`                          // Description of the integration.
+  Enabled                           *bool                                `json:"enabled,omitempty"`                              // Whether the integration is available to users.
+  Params                            *[]IntegrationParam                  `json:"params,omitempty"`                               // Array of params for the integration.
+  SupportedFormats                  *[]SupportedFormats                  `json:"supported_formats,omitempty"`                    // A list of data formats the integration supports. If unspecified, the default is all data formats. Valid values are: "txt", "csv", "inline_json", "json", "json_label", "json_detail", "json_detail_lite_stream", "xlsx", "html", "wysiwyg_pdf", "assembled_pdf", "wysiwyg_png", "csv_zip".


@kppk should this be []SupportedFormatType instead of []SupportedFormats? if so, then there's some misconfiguration between packages/sdk-codegen/src/sdkModels.ts and packages/sdk-codegen/src/go.gen.ts

joeldodge79 force-pushed the joeldodge/python-enum-bugfix branch 2 times, most recently from 992fef4 to 61b9645 Compare September 29, 2020 18:18

Fix array of enums

45e9966

A property that is an array of enum values was being generated as only that enum. This change corrects that.

joeldodge79 force-pushed the joeldodge/python-enum-bugfix branch from 61b9645 to 45e9966 Compare September 29, 2020 18:22

joeldodge79 commented Sep 29, 2020

View reviewed changes

joeldodge79 requested review from jkaster and josephaxisa and removed request for jkaster September 29, 2020 18:22

jkaster requested changes Sep 29, 2020

View reviewed changes

Removed csharp changes from this PR for later consideration

10e569e

jkaster approved these changes Sep 29, 2020

View reviewed changes

joeldodge79 merged commit ba9bb64 into master Sep 30, 2020

joeldodge79 deleted the joeldodge/python-enum-bugfix branch September 30, 2020 03:16

joeldodge79 commented Nov 18, 2020

View reviewed changes

This was referenced Mar 24, 2021

chore: release #530

Closed

chore: release main #532

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix array of enums #335

Fix array of enums #335

joeldodge79 commented Sep 29, 2020 •

edited

Loading

joeldodge79 left a comment

joeldodge79 Sep 29, 2020

jkaster Sep 29, 2020

joeldodge79 Sep 29, 2020

jkaster Sep 29, 2020

joeldodge79 Sep 29, 2020

jkaster Sep 29, 2020

joeldodge79 Sep 29, 2020

joeldodge79 Sep 29, 2020

jkaster Sep 29, 2020

joeldodge79 Sep 29, 2020

jkaster Sep 29, 2020

joeldodge79 Sep 29, 2020

joeldodge79 Sep 29, 2020

jkaster Sep 29, 2020

joeldodge79 Sep 29, 2020

jkaster Sep 29, 2020

jkaster left a comment

jkaster Sep 29, 2020

jkaster Sep 29, 2020

jkaster Sep 29, 2020

jkaster Sep 29, 2020

jkaster left a comment

joeldodge79 Nov 18, 2020

	const baseName = methodName ? titleCase(`${methodName}_${name}`) : name
	const baseName = methodName ? titleCase(`${methodName}_${name}`) : name
	baseName = (baseName in types) ? titleCase(`${parentTypeName}_${baseName}) : baseName

Fix array of enums #335

Fix array of enums #335

Conversation

joeldodge79 commented Sep 29, 2020 • edited Loading

joeldodge79 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkaster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joeldodge79 commented Sep 29, 2020 •

edited

Loading