-
Notifications
You must be signed in to change notification settings - Fork 349
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
fix: support mutliple options in snakeToCamel flag #429
Conversation
Changed `snakeToCamel: boolean` to `snakeToCamel: boolean | string`. So that `snakeToCamel` would look like `snakeToCamel: Array<"keys" | "json">`. So user can set true / false and then map those to true --> ["keys"] and false --> [] and then keys,json => ["keys", "json"] resolves stephenh#423
- With `--ts_proto_opt=snakeToCamel=false`, fields will be kept snake case. | ||
|
||
Defaults to `true`. | ||
- With `--ts_proto_opt=snakeToCamel=false`, fields will be kept snake case. `snakeToCamel` can also be set as string with `--ts_proto_opt=snakeToCamel=keys,json`. `keys` will keep field names as camelCase and `json` will keep json field names as camelCase. Empty string will keep field names as snake_case. |
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.
Nice!
@@ -30,7 +30,7 @@ export enum ServiceOption { | |||
|
|||
export type Options = { | |||
context: boolean; | |||
snakeToCamel: boolean; | |||
snakeToCamel: boolean | 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.
Ah yeah; this makes sense, although what might be nifty is to make this:
snakeToCamel: Array<'json' | 'keys'>;
So that the call sites like maybeSnakeToCamel
and do just options.snakeToCamel.includes('keys')
and then inside of optionsFromParamter
we can handle "boolean --> string[]" kinda like:
if ((options.snakeToCamel as any) === false) {
options.snakeToCamel = [];
} else if ((options.snakeToCamel as any) === true) {
options.snakeToCamel = ['keys', 'json'];
}
Which has a few as any
s which is a little weird, but we've done it for a few other options like this have that "might be boolean/might be strings".
Maybe try that? I think it will simplify the call sites quite a bit.
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.
@ajaykarthikr I'm going to go ahead and merge this and do ^ as a follow up. Thanks for the PR!
return field.jsonName; | ||
} | ||
return maybeSnakeToCamel(field.name, options); | ||
return stringToSnakeCase(field.name); |
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.
Ah yeah...I see what you're thinking here, but maybe just leave this out?
I'm thinking that if the user has set snakeToCamel=false
that's not exactly the same thing as "camelToSnake=true" i.e. requesting that we actively snake-ize fields in the JSON. So I'm tempted to just leave the field name as it is.
🎉 This PR is included in version 1.93.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Restores
snakeToCamel=false
functionality due the breaking change introduced by #210. Implements changes as per the discussion in #423.Now
snakeToCamel: boolean
issnakeToCamel: boolean | string
and accepts string input tookeys, json
.This is how the option will be mapped:
Resolves #423