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: Unbreak a use case for #1110 / fix for #1121 #1123

Merged
merged 1 commit into from
Oct 15, 2024

Conversation

halkeye
Copy link
Contributor

@halkeye halkeye commented Oct 15, 2024

"Function" is builtin, but anything prefixed or suffixed for whatever reason shouldn't be handled the same way. So check if the final result is in builtin list, not the original unmodified name

I have concerns about maybeSnakeToCamel being called earlier now, but i can't find any tests for options.snakeToCamel.includes("keys") so i'm not sure if i broke anything

  /** Converts `key` to TS/JS camel-case idiom, unless overridden not to. */
💡export function maybeSnakeToCamel(key: string, options: Pick<Options, "snakeToCamel">): string {
    if (options.snakeToCamel.includes("keys") && key.includes("_")) {
      return snakeToCamel(key);
    } else {
      return key;
    }
  }

But all tests pass

@halkeye halkeye changed the title Unbreak a use case for #1110 / fix for #1121 fix: Unbreak a use case for #1110 / fix for #1121 Oct 15, 2024
"Function" is builtin, but anything prefixed or suffixed for whatever
reason shouldn't be handled the same way. So check if the final result
is in builtin list, not the original unmodified name
const tsFullName = tsPrefix + maybeSnakeToCamel(messageName(message), options);
const tsFullNameWithAffixes = `${options.typePrefix}${tsFullName}${options.typeSuffix}`;
const tsFullName = tsPrefix + maybeSnakeToCamel(message.name, options);
const tsFullNameWithAffixes = messageName(`${options.typePrefix}${tsFullName}${options.typeSuffix}`);
Copy link
Owner

Choose a reason for hiding this comment

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

Nice! Great fix to just move where we check the builtInNames

@stephenh stephenh merged commit 476e99b into stephenh:main Oct 15, 2024
6 checks passed
@halkeye
Copy link
Contributor Author

halkeye commented Oct 15, 2024

🙏 can i get another label :P

@stephenh
Copy link
Owner

Of course! Added.

stephenh pushed a commit that referenced this pull request Oct 15, 2024
## [2.2.4](v2.2.3...v2.2.4) (2024-10-15)

### Bug Fixes

* Unbreak a use case for [#1110](#1110) / fix for [#1121](#1121) ([#1123](#1123)) ([476e99b](476e99b))
@stephenh
Copy link
Owner

🎉 This issue has been resolved in version 2.2.4 🎉

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants