Skip to content
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

[CLOSES #933] Add enum support to web-actions tab form #1220

Merged
merged 1 commit into from Oct 9, 2019

Conversation

@adrw
Copy link
Collaborator

commented Oct 9, 2019

Screen Shot 2019-10-09 at 13 11 11

@adrw adrw force-pushed the adrw:adrw/20191009.enum branch 3 times, most recently from 936b337 to db29ae0 Oct 9, 2019
@adrw adrw requested review from wesleyk and acostama Oct 9, 2019
Copy link
Collaborator

left a comment

Nice! LMK when an updated test case for RequestTypes is added

@@ -93,15 +93,34 @@ class RequestTypes {
// TODO: Support maps
fields.add(Field(fieldName, fieldClass.qualifiedName!!, repeated))
}
Enum::class -> {

This comment has been minimized.

Copy link
@wesleyk

wesleyk Oct 9, 2019

Collaborator

let's update RequestTypesTest

This comment has been minimized.

Copy link
@adrw

adrw Oct 9, 2019

Author Collaborator

Will do!

This comment has been minimized.

Copy link
@adrw

adrw Oct 9, 2019

Author Collaborator

Added!

Copy link
Collaborator

left a comment

This is great!

@adrw adrw force-pushed the adrw:adrw/20191009.enum branch from db29ae0 to 77f5eb8 Oct 9, 2019
@wesleyk
wesleyk approved these changes Oct 9, 2019
@@ -37,5 +37,8 @@ internal class RequestTypesTest {
// Check repeated types
assertThat(shipmentType.fields).contains(Field("notes", "String", true))
assertThat(warehouseType.fields).contains(Field("alternates", Warehouse::class.qualifiedName!!, true))

// Check enum types
assertThat(shipmentType.fields).contains(Field("status", "Enum<${Shipment.State::class.qualifiedName!!},${Shipment.State.values().joinToString(",")}>", false))

This comment has been minimized.

Copy link
@wesleyk

wesleyk Oct 9, 2019

Collaborator

/nit how about we actually assert the full values rather than programatically calculating it? I prefer that as being more assertive than having to reason about what the expression resolves to

@adrw adrw force-pushed the adrw:adrw/20191009.enum branch from 77f5eb8 to b5c619d Oct 9, 2019
@adrw adrw merged commit 3f28caf into cashapp:master Oct 9, 2019
3 checks passed
3 checks passed
ci/circleci: docs Your tests passed on CircleCI!
Details
ci/circleci: java Your tests passed on CircleCI!
Details
ci/circleci: node Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.