search

django-ftl / fluent-compiler

High performance Python implementation of Fluent, Mozilla's l10n language
Other
21 stars 4 forks source link

Make fluent_number obey format specifiers for currencies #5

Closed spookylukey closed 3 years ago

spookylukey commented 3 years ago

Copied from https://github.com/projectfluent/python-fluent/pull/157

Thanks @Sh4rK

spookylukey commented 3 years ago

Regarding @Pike's comment on the other PR:

As for this aspect of number formatting, I'm wary of exposing the number of digits to localizers for currencies. That doesn't sound like something that should be language specific. @stasm ?

Still, might be good to have a way for the programmer to control this.

In fluent-compiler, we have ftl_arg_spec which allows control over what args are available to FTL files. However, we don't (yet) have the ability to say “make maximumFractionDigits allowed if its not a currency, but disallowed if is a currency", which I think is what would be needed.

Sh4rK commented 3 years ago

Thanks!

We are actually using Fluent with Django, and hence using your package, so my original goal was to get the PR accepted "upstream", then open a PR here as well, but it seems you have read my mind :)

What's your policy regarding these kind of things? Are you willing to merge it even if upstream doesn't? Can I help in any way?

spookylukey commented 3 years ago

@Sh4rK I'm trying to keep compatibility as much as possible, so I opened this PR in anticipation of it getting merged there, and to see if it looks any different in this package. So thanks for opening the issue on python-fluent first, that really helps us keep in sync.

That said, I have been simply unable to use official python-fluent with its current feature set, especially due to a missing solution for HTML handling, which is implemented in fluent-compiler using the 'escapers' functionality. So I have been more ready to add things where it solves developer problems and makes the package usable now, rather than wait for 100% agreement on the path forward.

If you can cope with potential backwards incompatible changes in the future, I think this looks like a good change.

spookylukey commented 3 years ago

We are actually using Fluent with Django, and hence using your package

Forgot to say - cool, django-ftl has a user apart from me :smile: :champagne:

Sh4rK commented 3 years ago

I opened this PR in anticipation of it getting merged there

I opened that one a while ago, and unfortunately there's not much activity, so I'm starting to give up on that :/

If you can cope with potential backwards incompatible changes in the future, I think this looks like a good change.

Well we haven't ever used the other package (and truthfully only did a quick test with yours, but we plan to go forward with it), so it wouldn't really be backwards incompatible for us specifically :)

Forgot to say - cool, django-ftl has a user apart from me 😄 🍾

Thanks for creating it, it's exactly what we needed!

Sh4rK commented 3 years ago

Awesome!