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

fix: support mutliple options in snakeToCamel flag #429

Merged
merged 2 commits into from
Dec 13, 2021

Conversation

ajaykarthikr
Copy link
Collaborator

Restores snakeToCamel=false functionality due the breaking change introduced by #210. Implements changes as per the discussion in #423.

Now snakeToCamel: boolean is snakeToCamel: boolean | string and accepts string input too keys, json.

This is how the option will be mapped:

  • snakeToCamel=true (the default) --> ["keys", "json"]
  • snakeToCamel=false --> []
  • snakeToCamel=keys --> ["keys"], use camel cased message keys, but still snake case in to/from JSON
  • snakeToCamel=json --> ["json"], use proto3-spec camel casing in to/fromJSON, but still snake case in message keys
  • snakeToCamel=keys,json --> ["keys","json"]

Resolves #423

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.
Copy link
Owner

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;
Copy link
Owner

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 anys 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.

Copy link
Owner

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);
Copy link
Owner

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.

@stephenh stephenh merged commit cff6674 into stephenh:main Dec 13, 2021
stephenh pushed a commit that referenced this pull request Dec 13, 2021
## [1.93.3](v1.93.2...v1.93.3) (2021-12-13)

### Bug Fixes

* support mutliple options in snakeToCamel flag  ([#429](#429)) ([cff6674](cff6674)), closes [#423](#423)
@stephenh
Copy link
Owner

🎉 This PR is included in version 1.93.3 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

version 1.90.0 broke snakeToCamel=false functionality
2 participants