search

projectfluent / fluent-rs

Rust implementation of Project Fluent
https://projectfluent.org
Apache License 2.0
1.09k stars 98 forks source link

Add NUMBER builtin function #353

Closed Xiretza closed 6 months ago

Xiretza commented 7 months ago

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 commented 7 months ago

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

alerque commented 6 months ago

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

waywardmonkeys commented 6 months ago

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 commented 6 months ago

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: https://github.com/projectfluent/fluent-rs/blob/f6168c8101af7dc39dbfbed286d2b6f9e7271fd9/fluent-bundle/src/types/number.rs#L79

alerque commented 6 months ago

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.

Xiretza commented 6 months ago

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

alerque commented 6 months ago

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

waywardmonkeys commented 6 months ago

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

Xiretza commented 6 months ago

I've added an integration test.

nazar-pc commented 4 months ago

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 https://github.com/projectfluent/fluent-rs/issues/19 and https://github.com/projectfluent/fluent-rs/issues/313 closed as completed.

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