Skip to content

Stdlib: rename binary operations to match JavaScript terms #7353

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

Merged
merged 4 commits into from
Mar 25, 2025

Conversation

cometkim
Copy link
Member

@cometkim cometkim requested review from cknitt and zth March 22, 2025 15:19
@cometkim cometkim force-pushed the stdlib-rename-bops branch 2 times, most recently from bdeefad to ffb0149 Compare March 22, 2025 15:28
@cknitt
Copy link
Member

cknitt commented Mar 23, 2025

👍 for cleaning up the naming/adapt it to JS terms.

Some questions:

  • Why not keep the Bitwise submodule? Modules/applications that do a lot of bitwise operations might want to open just that submodule.
  • Wouldn't shiftLeft/shiftRight/shiftRightUnsigned be better naming than leftShift/rightShift/unsignedRightShift (e.g., for grouping these functions together in autocomplete)?
  • Do we need deprecations for the old functions (for those migrating from Core)?

@cometkim
Copy link
Member Author

I simply didn't think it was necessary to hide the bitwise operations, so I adjusted them to be consistent across multiple modules.

Wouldn't shiftLeft/shiftRight/shiftRightUnsigned be better naming than leftShift/rightShift/unsignedRightShift (e.g., for grouping these functions together in autocomplete)?

Yeah it could be. If it becomes a common convention on the Stdlib surface, it's worth following.

@cknitt
Copy link
Member

cknitt commented Mar 23, 2025

@zth What are your thoughts regarding function naming / the Bitwise submodule?

@cknitt
Copy link
Member

cknitt commented Mar 23, 2025

Yeah it could be. If it becomes a common convention on the Stdlib surface, it's worth following.

We already have reduce and reduceRight (not rightReduce), for example.

@cometkim cometkim force-pushed the stdlib-rename-bops branch from ffb0149 to 1bf704e Compare March 24, 2025 19:36
@cometkim cometkim force-pushed the stdlib-rename-bops branch from 1bf704e to 4d51173 Compare March 24, 2025 20:12
@cometkim cometkim force-pushed the stdlib-rename-bops branch from 4d51173 to 7ad3f30 Compare March 24, 2025 20:29
Copy link
Member

@cknitt cknitt left a comment

Choose a reason for hiding this comment

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

Thanks a lot! 👍

@cknitt
Copy link
Member

cknitt commented Mar 25, 2025

Ah, sorry, need to ask about a CHANGELOG entry again.

@cknitt
Copy link
Member

cknitt commented Mar 25, 2025

Merging as is, will add the CHANGELOG entry while preparing the release.

@cknitt cknitt merged commit 4603da8 into rescript-lang:master Mar 25, 2025
20 checks passed
@cknitt cknitt mentioned this pull request Mar 29, 2025
5 tasks
@cometkim cometkim deleted the stdlib-rename-bops branch March 31, 2025 11:40
fhammerschmidt pushed a commit that referenced this pull request Apr 4, 2025
* Stdlib: rename binary operations to match JavaScript terms

* rename shift operations

* remove old name

* update analysis result
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.

2 participants