-
Notifications
You must be signed in to change notification settings - Fork 377
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
feat: support customName
options
#585
Conversation
src/index.ts
Outdated
/** | ||
* Custom function to provide a type name for a given schema | ||
*/ | ||
customName?: (schema: LinkedJSONSchema, keyNameFromDefinition: string | undefined) => string |
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.
A bit different from the proposal in #181 (comment), I think it would be more useful to expose the schema object directly, as both id
and title
comes from the schema object. This would allow demands like #456 to be easily achieved from the userland.
src/parser.ts
Outdated
): string | undefined { | ||
if (options.customName) { | ||
return options.customName(schema, keyNameFromDefinition) |
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.
I wonder if it would be more useful to make it so customName
can return string | null
, then if the result is null
, we fall back to the logic below. That would make it so you can customize some names, but you don't have to handle very single name.
Would that be better/worse for your use case, for example?
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.
I would pretty much prefer to allow nullish returns to fallback. I didn't do it because I saw you mention it here: #215 (comment)
If you change your mind, I am happy to push an update
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.
Returning string | void
is fine -- I disagree with old-me :)
Feel free to update the PR and add a test case, and I'll go ahead and merge this in. Thanks!
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.
See comment
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.
Thanks for the contribution!
Redo #215
Fix #181
Different from #215, this PR does not include CLI support - since the options accept a function, I suppose it would be more useful for programmatic usages.