-
Notifications
You must be signed in to change notification settings - Fork 61
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
Typescript definitions for operations #224
Typescript definitions for operations #224
Conversation
…romise<undefined>
59c916f
to
cb3980a
Compare
templates/helpers/operation.js
Outdated
const getBodyModelNameInCamelCase = operation => { | ||
const MODELS_SHOULD_NOT_PROCESS = ['object', 'string', 'undefined']; | ||
|
||
// BEGIN work around spec mismatches and upstream parsing incosistencies |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: should be "inconsistencies", though I appreciate the irony
templates/helpers/operation.js
Outdated
} | ||
} | ||
return typedArgs.join(', '); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: missing semicolon
Several of the const
below as well
const MODELS_SHOULD_NOT_PROCESS = ['object', 'string', 'undefined']; | ||
|
||
// BEGIN work around spec mismatches and upstream parsing incosistencies | ||
const RESTRICTED_MODEL_PROPERTY_OVERRIDES = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good work dealing with the exceptions, but I'm worried the next person to touch this may not follow, particularly since the templates say to set the property UNLESS it is an override (the opposite of what I'd expect an override to do).
Could you drop in a 1-2 line comment clarifying the logic here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure - I was hoping that could be read as 'add a property unless it's an illegal override', but that didn't work out so well 😅
Generated model types are extracted to a separate branch: #225
TS lint setup is to follow in a subsequent PR.
Resolves: OKTA-291118