Python SDK: Enum Support #242

josephaxisa · 2020-06-22T20:18:23Z

Made model attributes of type enum string forward references with a registered structure hook
Added a unit test for a model containing an enum type attribute
Broke up structure_hook into reserved_kw_structure_hook and forward_ref_structure_hook

Enums are now fully implemented for Typescript, Swift, Kotlin, and C# Python needs a little bit more for full implementation, which will be completed in #242 * Added enum generation for all languages * Enums are not overwritten and are automatically renamed if the values for the enum vary from another enum of the same name * Miscellaneous - replacing `x-looker-values` with `enum` in converted OA spec - putting license statement into generated files using the `LICENSE` file contents in the package folder - made `commentHeader` a little smarter

joeldodge79

@josephaxisa and @jkaster

I've finally fixed our [de]serialization issues not only wrt to Enums but also uncovered and fixed a future bug: my commit on this branch is pretty big but what I want to point out is how the generated code will change a bit. I haven't made those changes yet but the first 180 lines of test_serialize.py is an example. It might be easiest to go over this with you guys in a meeting for better understanding.

joeldodge79 · 2020-06-24T14:29:27Z

python/tests/rtl/test_api_methods.py

-        ("/user", "https://self-signed.looker.com:19999/api/3.1/user"),
-        ("user", "https://self-signed.looker.com:19999/api/3.1/user"),
-        ("/user/1", "https://self-signed.looker.com:19999/api/3.1/user/1"),
-        ("user/1", "https://self-signed.looker.com:19999/api/3.1/user/1"),


now that I'm using cloudtop my base_url is http://joeldodge.c.googlers.com

jkaster

This looks ok to me but I'm not the person to approve it. I still need to fix my python config then I can at least test it.

jkaster · 2020-06-26T00:26:13Z

python/.python-version

 3.8.2
+3.7.6
+3.6.10


Does this change default python version selection?

sorry, missed this question: yes, the first one in the list is the default version when you cd into the directory. There are more than one version listed so that tox can find them and use them

jkaster · 2020-06-26T00:31:29Z

It might be easiest to go over this with you guys in a meeting for better understanding.

That would be fine with me!

jkaster

Minor typos, and a request to remove property sorting calls

jkaster · 2020-06-30T00:16:50Z

packages/sdk-codegen/src/codeGen.ts

-          props.push(this.declareProperty(bump, prop))
-        )
+        Object.values(type.properties)
+          .sort(this.sortProperties)


what's this sort call for?

I should have just closed this PR till it was ready to review. I was going to go through and annotate the PR with why I made certain changes, including this one:

In python I reverted to having all classes define their own __init__ for consistency and readability. That exposed a bug:

when relying on kw_only=True argument to the attr.s() class decorator, the order of required vs optional property declarations didn't matter. But now that we're not using that argument, and instead passing init=False - attr blows up saying that "optional declarations cannot come before required ones". So, in order for the python sdk to "compile" we need to make sure that all required properties are bubbled up to the top.

jkaster · 2020-06-30T00:18:12Z

packages/sdk-codegen/src/codeGen.ts

+  sortProperties(_a: IProperty, _b: IProperty) {
+    return 0
+  }


Did you intend to leave this in? I realize it's not sorting anything right now, but I don't think we should be sorting properties for type declarations. This destroys any intentional grouping/order the developer may have created.

yes, that was intended.

jkaster · 2020-06-30T00:21:29Z

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

+      for (const prop of Object.values(type.properties).sort(
+        this.sortProperties
+      )) {


We should not be sorting properties of a type.

let's talk about it - I can try to find another solution for python but that might be a bit of work

python/tests/rtl/test_serialize.py

added ICodeGen.typeProperties to produce the list of properties for declareType. The default is just type.properties but mypy/python requires that all required properties are declared before optional properties on a class so the python generator overrides this to provide the required properties first followed by the optional.

joeldodge79

@josephaxisa @jkaster I think this is ready for you to look over now. I've tried to leave explanatory comments on the PR to guide you. Feel free to push back on anything.

joeldodge79 · 2020-06-30T23:05:10Z

packages/sdk-codegen-scripts/src/convert.ts

@@ -213,7 +213,7 @@ export const convertSpec = (
    // patch to fix up small errors in source definition (not required, just to ensure smooth process)
    // indent no spaces
    // output to openApiFilename
-    run('swagger2openapi', [
+    run('yarn swagger2openapi', [


part of versionstamp bugfix

joeldodge79 · 2020-06-30T23:05:17Z

packages/sdk-codegen-scripts/src/fetchSpec.spec.ts

+  it('gets lookerVersion with supplied versions', async () => {
+    const actual = await fetchLookerVersion(props, {looker_release_version: '7.10.0'})
+    expect(actual).toEqual('7.10')
+  })
+


part of versionstamp bugfix

joeldodge79 · 2020-06-30T23:05:24Z

packages/sdk-codegen-scripts/src/fetchSpec.ts

+      return ''
    }
  }
-  return lookerVersion
+  const matches = versions.looker_release_version.match(/^\d+\.\d+/i)
+  return matches[0]


part of versionstamp bugfix

joeldodge79 · 2020-06-30T23:06:22Z

packages/sdk-codegen/src/codeGen.ts

@@ -235,7 +239,7 @@ export abstract class CodeGen implements ICodeGen {
        )
        propertyValues = props.join(this.enumDelimiter)
      } else {
-        Object.values(type.properties).forEach((prop) =>
+        this.typeProperties(type).forEach((prop) =>


this was my solution to allow custom behavior for language generators and keep the previous behavior the default. open to other ways if this doesn't seem right.

I think this looks reasonable

joeldodge79 · 2020-06-30T23:07:11Z

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

  describe('type creation', () => {
    it('with arrays and hashes', () => {
      const type = apiTestModel.types.Workspace
      const actual = gen.declareType(indent, type)
      expect(type.properties.id.type.name).toEqual('string')
      expect(actual).toEqual(`
-@attr.s(auto_attribs=True, kw_only=True)
+@attr.s(auto_attribs=True, init=False)


all models now have these arguments

joeldodge79 · 2020-06-30T23:41:29Z

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

+    if (usesReservedPythonKeyword) {
      this.hooks.push(
-        `sr.converter${this.apiRef}.register_structure_hook(\n${bump}${forwardRef},  # type: ignore\n${bump}${this.structureHook}  # type:ignore\n)`
+        `sr.converter${this.apiRef}.register_structure_hook(\n${bump}${type.name},  # type: ignore\n${bump}${this.structureHookTK}  # type:ignore\n)`


looks like we actually were registering the "translate_keys_structure_hook" functionality before, so LookmlModelExploreJoins probably was working, but now we're testing it :-)

tests are good!

joeldodge79 · 2020-06-30T23:42:30Z

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

@@ -441,11 +487,10 @@ ${this.hooks.join('\n')}
    const attrs: string[] = []
    const isEnum = type instanceof EnumType
    const baseClass = isEnum ? 'enum.Enum' : 'model.Model'
-    let attrsArgs = 'auto_attribs=True, kw_only=True'


now it's always auto_attribs=True, init=False so no need to have a separate variable

joeldodge79 · 2020-06-30T23:43:35Z

packages/sdk-codegen/src/sdkModels.ts

+  private filterRequiredProps(required: boolean) {
+    const filteredProps: PropertyList = {}
+    for (const key in this.properties) {
+      const prop = this.properties[key]
+      const condition = required ? prop.required : !prop.required
+      if (condition) {
+        filteredProps[key] = prop
+      }
+    }
+    return filteredProps
+  }
+
+  get requiredProperties() {
+    return this.filterRequiredProps(true)
+  }
+
+  get optionalProperties() {
+    return this.filterRequiredProps(false)
+  }
+


is this what you had in mind @jkaster ?

joeldodge79 · 2020-06-30T23:46:20Z

python/looker_sdk/rtl/serialize.py

+    data = _tr_data_keys(data)
+    actual_type = eval(forward_ref.__forward_arg__, context, locals())
+    if issubclass(actual_type, enum.Enum):
+        instance = converter.structure(data, actual_type)


handle enum: "Enum" type declarations in deserialization (well, with the help of the __annotations__ hack converting that to enum: ForwardRef("Enum"))

Sorry Python doesn't have a better built-in serialization solution!

oh, me too. Python was my gold standard scripting language for a while but the Typing implementation and this lack of easy/builtin class-instance/json serialization has disillusioned me. I'd say Typescript is quickly gaining ground for me except the huge barrier between "I wrote this typescript code" and "I want to run this typescript script" (even as a node script) is still daunting to me.

Yeah, the scripting in this repo is a bit awkward now, but if you look at the existing node scripts in the root package.json you can see the pattern that should always work.

joeldodge79 · 2020-06-30T23:50:55Z

yarn.lock

@@ -14144,7 +14144,7 @@ typedarray@^0.0.6:
  resolved "https://registry.yarnpkg.com/typedarray/-/typedarray-0.0.6.tgz#867ac74e3864187b1d3d47d996a78ec5c8830777"
  integrity sha1-hnrHTjhkGHsdPUfZlqeOxciDB3c=

-[email protected], typescript@^3.8.2, typescript@^3.8.3:
+[email protected], typescript@^3.6.0, typescript@^3.8.2, typescript@^3.8.3:


I have no idea why I have this change, should I discard it? maybe I have some old version of typescript on my cloudtop?

No, don't discard it. Let yarn.lock do what yarn.lock will

Resolved change request

jkaster

I just realized I responded to all your comments inline rather than doing a review!

Most of this looks good! We could do some very minor cleanup but there's nothing to hold up this PR, and this change and the other ones are clearly showing we need to do some better design for the low-level codegen components. Hopefully we can get to that before we do yet another language!

josephaxisa · 2020-07-01T13:25:38Z

I cannot start a review since I opened this PR, but this LGTM. Nice work Joel!

refactored serialization structure hooks to handle different types of ForwardRef objects (enum vs model object) Broke up structure_hook into reserved_kw_structure_hook and forward_ref_structure_hook fixed bug where "bare" ForwardRef annotations would bomb out on deserialization - doesn't exist in the wild yet but it probably will at some point - except that for now we always mark all API response model fields as optional because of the "fields" argument... fixed bug where "reserved_keyword_structure_hook" was not being applied to the top level Model type. reversed .python-version list so 3.8.2 is default added tox-pyenv because tox wasn't finding 3.6 on my box

jkaster

LGTM. Nothing blocking approval

jkaster · 2020-07-01T22:40:52Z

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

+  declareProperty(indent: string, property: IProperty, annotations = false) {
    const mappedType = this.typeMapModels(property.type)
    let propName = property.name
    if (this.pythonKeywords.has(propName)) {
      propName = propName + '_'
    }
    let propType = mappedType.name
    if (!property.required) {
-      propType = `Optional[${mappedType.name}] = ${this.nullStr}`
+      propType = `Optional[${mappedType.name}]`
+    }
+
+    let propDef
+    if (annotations) {
+      let annotation = propType
+      if (this.isBareForwardRef(property)) {
+        annotation = `ForwardRef(${propType})`
+      }
+      propDef = `${this.bumper(indent)}"${propName}": ${annotation}`
+    } else {
+      if (!property.required) {
+        propType += ` = ${this.nullStr}`
+      }
+      propDef = `${indent}${propName}: ${propType}`


LOL. Let's worry about it if it breaks :)

jkaster · 2020-07-01T22:42:16Z

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

+    // so we need to emit the `__annotations__` property
+    return (
+      prop.required &&
+      (prop.type.customType || prop.type instanceof EnumType) &&


Oh, EnumType should definitely have a truthy customType. If it's not, we should get around to fixing that.

I think it doesn't which is why I had to add the EnumType check in here.

jkaster · 2020-07-01T22:43:40Z

packages/sdk-codegen/src/sdkModels.ts

+  /**
+   * key/value collection of required properties for this type
+   */
+  requiredProperties: PropertyList


Do you need this as a list, or if it's just for natural order iteration, it could be IProperty[]

I wanted it to be the same type as properties - ICodeGen.typeProperties returns a IProperty[] because that's all declareType cares about is natural order iteration

Base automatically changed from jk/enums to master June 23, 2020 19:28

joeldodge79 force-pushed the jax/python-enums branch 6 times, most recently from 090b8d4 to 8a2f476 Compare June 25, 2020 23:12

joeldodge79 reviewed Jun 25, 2020

View reviewed changes

jkaster reviewed Jun 26, 2020

View reviewed changes

joeldodge79 force-pushed the jax/python-enums branch 2 times, most recently from 80bc42a to 5d80f7e Compare June 30, 2020 00:01

jkaster previously requested changes Jun 30, 2020

View reviewed changes

josephaxisa marked this pull request as ready for review June 30, 2020 09:20

joeldodge79 added 2 commits June 30, 2020 22:58

Bugfix version stamping

932c1a3

joeldodge79 force-pushed the jax/python-enums branch 2 times, most recently from 16464d2 to c952d16 Compare June 30, 2020 23:53

joeldodge79 reviewed Jun 30, 2020

View reviewed changes

joeldodge79 requested a review from jkaster June 30, 2020 23:55

jkaster previously approved these changes Jul 1, 2020

View reviewed changes

joeldodge79 dismissed jkaster’s stale review via 598a3ad July 1, 2020 22:11

joeldodge79 force-pushed the jax/python-enums branch from c952d16 to 598a3ad Compare July 1, 2020 22:11

joeldodge79 requested a review from jkaster July 1, 2020 22:11

joeldodge79 force-pushed the jax/python-enums branch from 598a3ad to a839630 Compare July 1, 2020 22:20

jkaster approved these changes Jul 1, 2020

View reviewed changes

joeldodge79 merged commit 1b64e1b into master Jul 2, 2020

joeldodge79 deleted the jax/python-enums branch July 2, 2020 00:08

This was referenced Mar 24, 2021

chore: release #530

Closed

chore: release main #532

Merged

 .8.2
+.7.6
+.6.10

Python SDK: Enum Support #242

Python SDK: Enum Support #242

Conversation

josephaxisa commented Jun 22, 2020

joeldodge79 left a 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

jkaster commented Jun 26, 2020

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

Choose a reason for hiding this comment

jkaster left a comment

Choose a reason for hiding this comment

josephaxisa commented Jul 1, 2020

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

Choose a reason for hiding this comment