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

Add NUMBER builtin function #353

Merged
merged 4 commits into from
May 7, 2024
Merged

Conversation

Xiretza
Copy link
Contributor

@Xiretza Xiretza commented Apr 26, 2024

Followup to #259. Adds the type property to FluentNumberOptions and makes it available through a new NUMBER() built-in fluent function. Built-ins (currently just NUMBER()) are added to the bundle using FluentBundle::add_builtins().

Cc #19.

@alerque
Copy link
Collaborator

alerque commented Apr 26, 2024

Will review, but definitely waiting on #347 before we can actually do anything about this ;-) Thanks for taking the time to contribute.

Copy link
Collaborator

@waywardmonkeys waywardmonkeys left a comment

Choose a reason for hiding this comment

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

Please rebase and change the derivation of Default once #356 lands.

Are there other tests that should be added?

fluent-bundle/src/types/number.rs Outdated Show resolved Hide resolved
@alerque alerque force-pushed the number-function branch from a3a63c0 to 29243fb Compare May 6, 2024 07:52
@alerque
Copy link
Collaborator

alerque commented May 6, 2024

I was reviewing this and went ahead and rebased to fix the commit messages and also threw in the #[derive(Default) cleanup too.

@waywardmonkeys
Copy link
Collaborator

A couple of notes:

In #259, @gregtatum mentioned using kind rather than r#type ... not sure if that's relevant here or not.

Also, https://projectfluent.org/fluent/guide/functions.html#number lists a number of other options. Should any of them be supported as part of this? (or a follow-up issue filed to track that they're missing?)

@Xiretza
Copy link
Contributor Author

Xiretza commented May 6, 2024

In #259, @gregtatum mentioned using kind rather than r#type ... not sure if that's relevant here or not.

I changed that back because I wanted to avoid using a different term in Rust than is used everywhere else.

Also, https://projectfluent.org/fluent/guide/functions.html#number lists a number of other options. Should any of them be supported as part of this? (or a follow-up issue filed to track that they're missing?)

Those should already be handled here:

pub fn merge(&mut self, opts: &FluentArgs) {

@alerque
Copy link
Collaborator

alerque commented May 6, 2024

I would have preferred "kind" too, but given that the Guide and Python both already use "type" I do agree that we should stick with that for now. If changing this is a priority we should hurry up and suggest the fix in the guide and spec, then implement it that way in Rust and Python can take their time about adapting. Honestly I'm not sure it's worth it just to avoid a reserved word we can easily use raw strings for anyway.

Copy link
Collaborator

@alerque alerque left a comment

Choose a reason for hiding this comment

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

I think I'm on board with this and most discussion is resolved, but I have to agree with this one:

Are there other tests that should be added?

How is this getting tested?

@Xiretza
Copy link
Contributor Author

Xiretza commented May 6, 2024

There's a doctest, I can add integration tests too if desired.

Copy link
Collaborator

@alerque alerque left a comment

Choose a reason for hiding this comment

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

I saw the example in the docstring but I'd missed the output in the test run. I just confirmed it is there and don't know specifically what else, if anything, would need a test.

@alerque alerque enabled auto-merge May 6, 2024 13:58
@alerque
Copy link
Collaborator

alerque commented May 6, 2024

Back to you @waywardmonkeys. Was there something else specific you thought should be tested?

@alerque alerque added enhancement crate:fluent-bundle Issues related to fluent-bundle crate labels May 6, 2024
@waywardmonkeys
Copy link
Collaborator

I'd probably go with something like the Python tests: https://github.com/projectfluent/python-fluent/pull/193/files#diff-30d1a24a3e67774b5d192acb1cd3f532961bae49746db23ab7b7962e7cb8a25d

auto-merge was automatically disabled May 6, 2024 16:51

Head branch was pushed to by a user without write access

@Xiretza
Copy link
Contributor Author

Xiretza commented May 6, 2024

I've added an integration test.

@alerque alerque force-pushed the number-function branch from 119a0f8 to 1984162 Compare May 6, 2024 17:09
@alerque alerque enabled auto-merge May 6, 2024 17:11
@alerque alerque requested a review from waywardmonkeys May 6, 2024 20:07
@alerque alerque merged commit c904ac3 into projectfluent:main May 7, 2024
6 checks passed
bors referenced this pull request in rust-lang-ci/rust May 21, 2024
@nazar-pc
Copy link
Contributor

nazar-pc commented Aug 3, 2024

I just tried main and still getting Unknown function: NUMBER(), am I missing something?

Also maximumFractionDigits doesn't seem to be supported as described in the docs, am I right?

This is all a bit confusing, especially with #19 and #313 closed as completed.

UPD: I didn't call add_builtins, why would it not be used by default?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
crate:fluent-bundle Issues related to fluent-bundle crate enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants