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 inconsistencies with serde and add SCREAMING-KEBAB-CASE #298

Merged
merged 5 commits into from
Apr 9, 2024

Conversation

escritorio-gustavo
Copy link
Contributor

@escritorio-gustavo escritorio-gustavo commented Apr 5, 2024

Goal

This PR aims to fix a few inconsistencies with serde caused by our use of inflector:

  • Fix snake_case inconsistency
  • Fix SCREAMING_SNAKE_CASE inconsistency
  • Fix kebab-case inconsistency
  • Add SCREMING-KEBAB-CASE
  • Remove dependency on inflector
  • serde only accepts the rename_all rule if the case is written exactly as described (i.e. CAMELCASE, Kebab-case, etc. are not allowed), where as ts accepts any changes in case, dashes and underscores. This was changed for consistency with serde

Closes #297

Changes

  • Manually implement snake_case like in serde
  • Change parse_assign_inflection to only accept cases written strictly like in the docs
  • Add Inflection::ScreamingKebab
  • Remove dependency on inflector

Checklist

  • I have followed the steps listed in the Contributing guide.
  • If necessary, I have added documentation related to the changes made.
  • I have added or updated the tests related to the changes made.

@escritorio-gustavo
Copy link
Contributor Author

escritorio-gustavo commented Apr 5, 2024

serde only accepts the rename_all rule if the case is written exactly as described (i.e. CAMELCASE, Kebab-case, etc. are not allowed), where as ts accepts any changes in case, dashes and underscores. This was changed for consistency with serde

@NyxCode this is the only "unnecessary" change here, though I think it's beneficial, as it forces the user to use the case they are setting rename_all to, which helps make sure they are using the correct one (e.g.: a common mistake is using CamelCase instead of PascalCase). Since the changes to snake_case, kebab-case and SCREAMING_SNAKE_CASE make this whole PR a breaking change, I thought it'd be a good time to change this.

Still, this is an extra breaking change, let me know if you want me to revert it

@escritorio-gustavo escritorio-gustavo added the bug Something isn't working label Apr 5, 2024
@NyxCode
Copy link
Collaborator

NyxCode commented Apr 8, 2024

Nice change, thanks!
I'm not sure regarding the breaking change, but I think it's alright, since we publish major versions relatively often anyway.

Copy link
Collaborator

@NyxCode NyxCode left a comment

Choose a reason for hiding this comment

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

great stuff!

@escritorio-gustavo escritorio-gustavo merged commit 1079b99 into main Apr 9, 2024
14 checks passed
@escritorio-gustavo escritorio-gustavo deleted the screaming_kebab_case branch April 9, 2024 11:07
@escritorio-gustavo escritorio-gustavo added the breaking change This PR will change the public API in some way label Apr 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change This PR will change the public API in some way bug Something isn't working
Projects
None yet
2 participants