Skip to content

Python SDK: Enum Support #242

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 2, 2020
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/sdk-codegen-scripts/src/convert.ts
Original file line number Diff line number Diff line change
@@ -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', [
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

part of versionstamp bugfix

swaggerFilename,
'-p',
'-i',
5 changes: 5 additions & 0 deletions packages/sdk-codegen-scripts/src/fetchSpec.spec.ts
Original file line number Diff line number Diff line change
@@ -60,6 +60,11 @@ describe('fetch operations', () => {
expect(actual).not.toEqual('')
})

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

Comment on lines +63 to +67
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

part of versionstamp bugfix

it('gets version info', async () => {
expect(props).toBeDefined()
const version = await getVersionInfo(props)
7 changes: 3 additions & 4 deletions packages/sdk-codegen-scripts/src/fetchSpec.ts
Original file line number Diff line number Diff line change
@@ -289,17 +289,16 @@ export const fetchLookerVersion = async (
versions?: any,
options?: Partial<ITransportSettings>
) => {
let lookerVersion = ''
if (!versions) {
try {
versions = await fetchLookerVersions(props, options)
const matches = versions.looker_release_version.match(/^\d+\.\d+/i)
lookerVersion = matches[0]
} catch (e) {
warn(`Could not retrieve looker release version: ${e.message}`)
return ''
}
}
return lookerVersion
const matches = versions.looker_release_version.match(/^\d+\.\d+/i)
return matches[0]
Comment on lines +297 to +301
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

part of versionstamp bugfix

}

/**
6 changes: 5 additions & 1 deletion packages/sdk-codegen/src/codeGen.ts
Original file line number Diff line number Diff line change
@@ -223,6 +223,10 @@ export abstract class CodeGen implements ICodeGen {
return this.itself ? `${this.itself}.${value}` : value
}

typeProperties(type: IType) {
return Object.values(type.properties)
}

declareType(indent: string, type: IType) {
const bump = this.bumper(indent)
const props: string[] = []
@@ -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) =>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this looks reasonable

props.push(this.declareProperty(bump, prop))
)
propertyValues = props.join(this.propDelimiter)
232 changes: 216 additions & 16 deletions packages/sdk-codegen/src/python.gen.spec.ts
Original file line number Diff line number Diff line change
@@ -50,11 +50,17 @@ describe('python generator', () => {
it('has a models prologue', () => {
expect(gen.modelsPrologue('')).toEqual(`
# NOTE: Do not edit this file generated by Looker SDK Codegen
import attr
import datetime
import enum
from typing import Any, MutableMapping, Optional, Sequence

try:
from typing import ForwardRef # type: ignore
except ImportError:
from typing import _ForwardRef as ForwardRef # type: ignore

import attr

Comment on lines +57 to +63
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

need this in the prologue now that we use ForwardRef in some __annotations__ in model declarations

from looker_sdk.rtl import model
from looker_sdk.rtl import serialize as sr

@@ -76,21 +82,26 @@ class LookerSDK(api_methods.APIMethods):
`)
})
it('has a models epilogue', () => {
const type = apiTestModel.types.LookmlModelExploreJoins
gen.declareType(indent, type)
expect(gen.modelsEpilogue('')).toEqual(`

# The following cattrs structure hook registrations are a workaround
# for https://github.com/Tinche/cattrs/pull/42 Once this issue is resolved
# these calls will be removed.

import functools # noqa:E402
from typing import Any
try:
from typing import ForwardRef # type: ignore
except ImportError:
from typing import _ForwardRef as ForwardRef # type: ignore

structure_hook = functools.partial(sr.structure_hook, globals(), sr.converter)

forward_ref_structure_hook = functools.partial(sr.forward_ref_structure_hook, globals(), sr.converter)
translate_keys_structure_hook = functools.partial(sr.translate_keys_structure_hook, sr.converter)
sr.converter.register_structure_hook(
ForwardRef("LookmlModelExploreJoins"), # type: ignore
forward_ref_structure_hook # type:ignore
)
sr.converter.register_structure_hook(
LookmlModelExploreJoins, # type: ignore
translate_keys_structure_hook # type:ignore
)
Comment on lines +97 to +104
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

running gen.declareType above so that we can make sure we're registering the translate_keys_structure_hook on models. We weren't before and I think there was a silent bug with LookmlModelExploreJoins where from_/"from" was getting lost in translation...

the unittests passed because we explicitly register this structure hook on "Model"

`)
})
it('does not have a methods epilogue', () => {
@@ -450,14 +461,13 @@ return response`
})
})

// TODO reinstate these tests after package dependencies are updated correctly
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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

all models now have these arguments

class Workspace(model.Model):
"""
Attributes:
@@ -467,14 +477,159 @@ class Workspace(model.Model):
"""
can: Optional[MutableMapping[str, bool]] = None
id: Optional[str] = None
projects: Optional[Sequence["Project"]] = None`)
projects: Optional[Sequence["Project"]] = None

def __init__(self, *,
can: Optional[MutableMapping[str, bool]] = None,
id: Optional[str] = None,
projects: Optional[Sequence["Project"]] = None):
self.can = can
self.id = id
self.projects = projects`)
Comment on lines +482 to +488
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and all models now have a constructor

})
it('re-orders required props to top', () => {
const type = apiTestModel.types.CreateDashboardFilter
const actual = gen.declareType(indent, type)
expect(actual).toEqual(`
@attr.s(auto_attribs=True, init=False)
class CreateDashboardFilter(model.Model):
"""
Attributes:
dashboard_id: Id of Dashboard
name: Name of filter
title: Title of filter
type: Type of filter: one of date, number, string, or field
id: Unique Id
default_value: Default value of filter
model: Model of filter (required if type = field)
explore: Explore of filter (required if type = field)
dimension: Dimension of filter (required if type = field)
field: Field information
row: Display order of this filter relative to other filters
listens_to_filters: Array of listeners for faceted filters
allow_multiple_values: Whether the filter allows multiple filter values
required: Whether the filter requires a value to run the dashboard
ui_config: The visual configuration for this filter. Used to set up how the UI for this filter should appear.
"""
dashboard_id: str
name: str
title: str
type: str
id: Optional[str] = None
Comment on lines +514 to +518
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was both mypy as well as the attr runtime that complained about actual class level "optional before required" ordering.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good to know

default_value: Optional[str] = None
model: Optional[str] = None
explore: Optional[str] = None
dimension: Optional[str] = None
field: Optional[MutableMapping[str, Any]] = None
row: Optional[int] = None
listens_to_filters: Optional[Sequence[str]] = None
allow_multiple_values: Optional[bool] = None
required: Optional[bool] = None
ui_config: Optional[MutableMapping[str, Any]] = None

def __init__(self, *,
dashboard_id: str,
name: str,
title: str,
type: str,
id: Optional[str] = None,
default_value: Optional[str] = None,
model: Optional[str] = None,
explore: Optional[str] = None,
dimension: Optional[str] = None,
field: Optional[MutableMapping[str, Any]] = None,
row: Optional[int] = None,
listens_to_filters: Optional[Sequence[str]] = None,
allow_multiple_values: Optional[bool] = None,
required: Optional[bool] = None,
ui_config: Optional[MutableMapping[str, Any]] = None):
self.dashboard_id = dashboard_id
self.name = name
self.title = title
self.type = type
self.id = id
self.default_value = default_value
self.model = model
self.explore = explore
self.dimension = dimension
self.field = field
self.row = row
self.listens_to_filters = listens_to_filters
self.allow_multiple_values = allow_multiple_values
self.required = required
self.ui_config = ui_config`)
})
it('with translated keywords', () => {
const type = apiTestModel.types.LookmlModelExploreJoins
const actual = gen.declareType(indent, type)
// note the "from_" property
expect(actual).toEqual(`
@attr.s(auto_attribs=True, init=False)
class LookmlModelExploreJoins(model.Model):
"""
Attributes:
name: Name of this join (and name of the view to join)
dependent_fields: Fields referenced by the join
fields: Fields of the joined view to pull into this explore
foreign_key: Name of the dimension in this explore whose value is in the primary key of the joined view
from_: Name of view to join
outer_only: Specifies whether all queries must use an outer join
relationship: many_to_one, one_to_one, one_to_many, many_to_many
required_joins: Names of joins that must always be included in SQL queries
sql_foreign_key: SQL expression that produces a foreign key
sql_on: SQL ON expression describing the join condition
sql_table_name: SQL table name to join
type: The join type: left_outer, full_outer, inner, or cross
view_label: Label to display in UI selectors
"""
name: Optional[str] = None
dependent_fields: Optional[Sequence[str]] = None
fields: Optional[Sequence[str]] = None
foreign_key: Optional[str] = None
from_: Optional[str] = None
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's make sure our keyword translation generation is happening

outer_only: Optional[bool] = None
relationship: Optional[str] = None
required_joins: Optional[Sequence[str]] = None
sql_foreign_key: Optional[str] = None
sql_on: Optional[str] = None
sql_table_name: Optional[str] = None
type: Optional[str] = None
view_label: Optional[str] = None

def __init__(self, *,
name: Optional[str] = None,
dependent_fields: Optional[Sequence[str]] = None,
fields: Optional[Sequence[str]] = None,
foreign_key: Optional[str] = None,
from_: Optional[str] = None,
outer_only: Optional[bool] = None,
relationship: Optional[str] = None,
required_joins: Optional[Sequence[str]] = None,
sql_foreign_key: Optional[str] = None,
sql_on: Optional[str] = None,
sql_table_name: Optional[str] = None,
type: Optional[str] = None,
view_label: Optional[str] = None):
self.name = name
self.dependent_fields = dependent_fields
self.fields = fields
self.foreign_key = foreign_key
self.from_ = from_
self.outer_only = outer_only
self.relationship = relationship
self.required_joins = required_joins
self.sql_foreign_key = sql_foreign_key
self.sql_on = sql_on
self.sql_table_name = sql_table_name
self.type = type
self.view_label = view_label`)
})
it('with refs, arrays and nullable', () => {
const type = apiTestModel.types.ApiVersion
expect(type.properties.looker_release_version.type.name).toEqual('string')
const actual = gen.declareType(indent, type)
expect(actual).toEqual(`
@attr.s(auto_attribs=True, kw_only=True)
@attr.s(auto_attribs=True, init=False)
class ApiVersion(model.Model):
"""
Attributes:
@@ -484,7 +639,15 @@ class ApiVersion(model.Model):
"""
looker_release_version: Optional[str] = None
current_version: Optional["ApiVersionElement"] = None
supported_versions: Optional[Sequence["ApiVersionElement"]] = None`)
supported_versions: Optional[Sequence["ApiVersionElement"]] = None

def __init__(self, *,
looker_release_version: Optional[str] = None,
current_version: Optional["ApiVersionElement"] = None,
supported_versions: Optional[Sequence["ApiVersionElement"]] = None):
self.looker_release_version = looker_release_version
self.current_version = current_version
self.supported_versions = supported_versions`)
})

function checkMappedType(
@@ -513,6 +676,43 @@ class PermissionType(enum.Enum):
expect(actual).toEqual(expected)
})

it('needs __annotations__', () => {
const type = apiTestModel.types.RequiredResponseWithSingleEnum
expect(type).toBeDefined()
const actual = gen.declareType('', type)
const expected = `
@attr.s(auto_attribs=True, init=False)
class RequiredResponseWithSingleEnum(model.Model):
"""
Attributes:
query_id: Id of query to run
result_format: Desired async query result format. Valid values are: "inline_json", "json", "json_detail", "json_fe", "csv", "html", "md", "txt", "xlsx", "gsxml".
user:
roles: Roles assigned to group
"""
query_id: int
result_format: "ResultFormat"
user: "UserPublic"
roles: Optional[Sequence["Role"]] = None
__annotations__ = {
"query_id": int,
"result_format": ForwardRef("ResultFormat"),
"user": ForwardRef("UserPublic"),
Comment on lines +694 to +700
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

only classes that have these "bare forward ref" types need the __annotations__ correction

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really appreciate all the comments. Very helpful!

"roles": Optional[Sequence["Role"]]
}

def __init__(self, *,
query_id: int,
result_format: "ResultFormat",
user: "UserPublic",
roles: Optional[Sequence["Role"]] = None):
self.query_id = query_id
self.result_format = result_format
self.user = user
self.roles = roles`
expect(actual).toEqual(expected)
})

it('input models', () => {
// TODO this side-effect should NOT be required for the test
// type declarations should be atomic w/o side-effect requirements
@@ -536,7 +736,7 @@ class PermissionType(enum.Enum):
})
const actual = gen.declareType(indent, inputType)
expect(actual).toEqual(`
@attr.s(auto_attribs=True, kw_only=True, init=False)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cruft: kw_only is ignored when init=False is supplied

@attr.s(auto_attribs=True, init=False)
class WriteMergeQuery(model.Model):
"""
Dynamically generated writeable type for MergeQuery removes properties:
@@ -578,7 +778,7 @@ can, id, result_maker_id
const childInputType = apiTestModel.types.MergeQuerySourceQuery
const childActual = gen.declareType(indent, childInputType)
expect(childActual).toEqual(`
@attr.s(auto_attribs=True, kw_only=True, init=False)
@attr.s(auto_attribs=True, init=False)
class MergeQuerySourceQuery(model.Model):
"""
Attributes:
@@ -601,7 +801,7 @@ class MergeQuerySourceQuery(model.Model):
const grandChildInputType = apiTestModel.types.MergeFields
const grandChildActual = gen.declareType(indent, grandChildInputType)
expect(grandChildActual).toEqual(`
@attr.s(auto_attribs=True, kw_only=True, init=False)
@attr.s(auto_attribs=True, init=False)
class MergeFields(model.Model):
"""
Attributes:
Loading