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

Revert BuiltinFnName #151

Merged
merged 2 commits into from
Dec 7, 2022
Merged

Revert BuiltinFnName #151

merged 2 commits into from
Dec 7, 2022

Conversation

odashi
Copy link
Collaborator

@odashi odashi commented Dec 6, 2022

Overview

This change reverts BuiltinFnName enum.

CC: @ZibingZhang

Details

BuiltinFnName was introduced by #130 to constrain keys in BUILTIN_FUNCS. I originally thought that it was useful, but after several development I realized that using raw string is more desirable:

  • Enum introduces a strong type constraint. If I introduced mypy in my local environment, BUILTIN_FUNCS rejects all strings that are not defined in the BuiltinFnName, which is not a desirable behavior.
  • We are required to write very long ientifiers, e.g., constants.BuiltInFnName.EXP.value rather than "exp". The identifier also involves the same string EXP with the function name, meaning that we eventually need to write the function name with longer prefix every time.
  • The enum is rarely used: in many cases the current implementation keeps the string representation.

References

  • NA

Blocked by

  • NA

Copy link
Contributor

@ZibingZhang ZibingZhang left a comment

Choose a reason for hiding this comment

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

LGTM

@odashi odashi mentioned this pull request Dec 6, 2022
Copy link
Collaborator

@ShigekiKarita ShigekiKarita left a comment

Choose a reason for hiding this comment

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

Nice simplification.

@odashi odashi merged commit 02116c1 into main Dec 7, 2022
@odashi odashi deleted the revert_builtin branch December 7, 2022 03:59
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.

3 participants