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

Use field_name?: field_type for optional fields, instead of OneOf extension, see #5 #13

Merged
merged 1 commit into from
Apr 27, 2024

Conversation

dpup
Copy link
Owner

@dpup dpup commented Apr 26, 2024

Hello @seanami,

The tests passed without any changes, which is reassuring. Here's what the
generated code looks like:

With emit_unpopulated=true

export type OptionalFieldsResponse = {
  emptyStr: string
  emptyNumber: number
  emptyMsg: OptionalFieldsSubMsg | null
  emptyOptStr?: string
  emptyOptNumber?: number
  emptyOptMsg?: OptionalFieldsSubMsg | null
  zeroStr: string
  zeroNumber: number
  zeroMsg: OptionalFieldsSubMsg | null
  zeroOptStr?: string
  zeroOptNumber?: number
  zeroOptMsg?: OptionalFieldsSubMsg
  definedStr: string
  definedNumber: number
  definedMsg: OptionalFieldsSubMsg | null
  definedOptStr?: string
  definedOptNumber?: number
  definedOptMsg?: OptionalFieldsSubMsg | null
}

export type OptionalFieldsSubMsg = {
  str: string
  optStr?: string
}

And with emit_unpopulated=false:

export type OptionalFieldsResponse = {
  emptyStr?: string
  emptyNumber?: number
  emptyMsg?: OptionalFieldsSubMsg
  emptyOptStr?: string
  emptyOptNumber?: number
  emptyOptMsg?: OptionalFieldsSubMsg
  zeroStr?: string
  zeroNumber?: number
  zeroMsg?: OptionalFieldsSubMsg
  zeroOptStr?: string
  zeroOptNumber?: number
  zeroOptMsg?: OptionalFieldsSubMsg
  definedStr?: string
  definedNumber?: number
  definedMsg?: OptionalFieldsSubMsg
  definedOptStr?: string
  definedOptNumber?: number
  definedOptMsg?: OptionalFieldsSubMsg
}

export type OptionalFieldsSubMsg = {
  str?: string
  optStr?: string
}

Please review the following commits I made in branch dan/optional-again:

7444de9 (2024-04-26 16:07:33 -0700)
Use field_name?: field_type for optional fields, instead of OneOf extension, see #5

Code review reminders, by giving a LGTM you attest that:

  • Commits are adequately tested
  • Code is easy to understand and conforms to style guides
  • Incomplete code is marked with TODOs
  • Code is suitably instrumented with logging and metrics

@dpup dpup requested a review from seanami April 26, 2024 23:08
Copy link
Collaborator

@seanami seanami left a comment

Choose a reason for hiding this comment

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

Nice! 👍 It's encouraging how easy this was to modify. I think this is the way. The IsOneOf for optional fields seems like an internal implementation detail anyway, and it's not necessary for TypeScript's type system anyway. This will make the resulting TS types much nicer to work with in editors.

@dpup dpup merged commit 8206ee7 into main Apr 27, 2024
2 checks passed
@dpup dpup deleted the dan/optional-again branch April 27, 2024 17:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants