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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: camelCase to snake_case 馃悕 #6702

Merged
merged 6 commits into from
May 27, 2024

Conversation

aspeddro
Copy link
Contributor

@aspeddro aspeddro commented Mar 30, 2024

Node script https://github.com/aspeddro/rescript-camelCase-to-snake_case

TODO:

  • Fix comments
  • Update CHANGELOG
  • Add coding style in CONTRUBUTING.md

@aspeddro aspeddro marked this pull request as ready for review March 30, 2024 05:08
jscomp/gentype/EmitJs.ml Outdated Show resolved Hide resolved
jscomp/gentype/EmitJs.ml Outdated Show resolved Hide resolved
jscomp/gentype/EmitType.ml Outdated Show resolved Hide resolved
jscomp/gentype/EmitType.ml Outdated Show resolved Hide resolved
jscomp/gentype/EmitType.ml Outdated Show resolved Hide resolved
@cknitt
Copy link
Member

cknitt commented May 26, 2024

We discussed this at the retreat and agreed that this is indeed the way to go:

  • snake_case for OCaml code
  • camelCase for ReScript code

@aspeddro Could you rebase, add a CHANGELOG entry, and also add a corresponding note regarding coding style in CONTRIBUTING.md?

@aspeddro
Copy link
Contributor Author

Done

@cknitt
Copy link
Member

cknitt commented May 27, 2024

@aspeddro Thanks a lot!

I think we should get this merged soon, otherwise it will need to be rebased constantly because of conflicts.

To me it seems fine (plus it builds and tests succeed), but the changes are really a lot.
So could you maybe also have a look @zth @cristianoc (and whoever else would like to)?

@cristianoc
Copy link
Collaborator

@aspeddro Thanks a lot!

I think we should get this merged soon, otherwise it will need to be rebased constantly because of conflicts.

To me it seems fine (plus it builds and tests succeed), but the changes are really a lot. So could you maybe also have a look @zth @cristianoc (and whoever else would like to)?

Looks fine at a glance.
I guess if the only changes are renamings, and the output is identical, this should be safe, and we can as well merge before having more conflicts.

@cknitt cknitt merged commit cc42052 into rescript-lang:master May 27, 2024
19 checks passed
@cknitt
Copy link
Member

cknitt commented May 27, 2024

I guess if the only changes are renamings, and the output is identical, this should be safe, and we can as well merge before having more conflicts.

Agreed. Merged!

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

Successfully merging this pull request may close these issues.

None yet

3 participants