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

Allow overriding Fluent functions #14

Merged
merged 1 commit into from
Jul 8, 2023

Conversation

hakastein
Copy link
Contributor

Original fluent does not limit function overriding

  1. Function overriding allowed
  2. FluentBundle no more final

This is especially significant due to the lack of a built-in DATETIME function.

@hakastein hakastein force-pushed the feature/allow-override-functions branch from fa723a4 to 3e2d30c Compare June 6, 2023 15:07
@hakastein
Copy link
Contributor Author

So, what are you thinking about this request? In the original implementation, there were no limits (https://github.com/projectfluent/fluent.js/blob/main/fluent-bundle/src/bundle.ts#LL75C20-L75C20), and we successfully overwrote built-in functions to support square/cubic units and some messy formats like usd-per-square-meter to support $1000/m².

@hakastein
Copy link
Contributor Author

Hello, do you plan accept this? Or do you have any questions?

@jrmajor
Copy link
Owner

jrmajor commented Jun 19, 2023

Hi, I believe that as DATETIME and NUMBER are built-in functions, they're expected to work as described in Fluent Guide. Allowing to override them breaks this assumption.

Would defining a separate function, e.g. MY_NUMBER, work for your use case? Or are there any additional reasons to allow function overriding, which I haven't considered?

@hakastein
Copy link
Contributor Author

hakastein commented Jun 19, 2023

Fluent is great for its extensibility. And the original library does not impose any restrictions on extending and redefining functions. I don't see anything wrong with overriding/extending built-in functions. Especially given that basic support for Intl.NumberFormat is only planned in the future tc39/ecma402#767

So in FronEnd we just replace built-in function and its work like a charm.

@hakastein
Copy link
Contributor Author

So all this is necessary for uniform behavior. To make the backend implementation behave the same as the frontend implementation

Copy link
Owner

@jrmajor jrmajor left a comment

Choose a reason for hiding this comment

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

Fair enough, we can allow it for the sake of compatibility with fluent.js.

Please adjust your changes to the comments I've left and I will merge it.

src/Bundle/FluentBundle.php Outdated Show resolved Hide resolved
src/Bundle/FluentBundle.php Outdated Show resolved Hide resolved
@jrmajor jrmajor changed the title Feature/allow override functions Allow overriding Fluent functions Jun 19, 2023
@hakastein
Copy link
Contributor Author

Done

@hakastein hakastein requested a review from jrmajor June 30, 2023 11:50
@jrmajor jrmajor force-pushed the feature/allow-override-functions branch from 78adf22 to 6d216b7 Compare July 8, 2023 12:19
@jrmajor jrmajor merged commit 0dbb570 into jrmajor:master Jul 8, 2023
@jrmajor
Copy link
Owner

jrmajor commented Jul 8, 2023

@hakastein Thank you, and sorry for the delay. I will try to release a new version soon :)

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