Add new format filter

[shopmike]

Proposed new addition for a number format filter. Resolves outstanding issues and really complex workarounds (https://gist.github.com/hexerei/5bd632b2a179717e219fbe18c5793181)

Usage

{{ price | format: decimal_places, thousands_separator, decimal_separator}}

Examples

{{ 20 | format: 2}} -> 20.00
{{ 2000000 | format }} -> 2 000 000.00
{{ 2000000 | format: 0, '' }} -> 2000000
{{ 2000000 | format: 2, '#', '@' }} -> 2#000#000@00

Resolves

https://github.com/Shopify/liquid/issues/1087 https://github.com/Shopify/liquid/issues/567 https://github.com/Shopify/liquid/pull/1104 https://github.com/Shopify/liquid/pull/702

@pushrax @ashmaroli @fw42 @Shopify/guardians-of-the-liquid @Shopify/liquid

[tobi]

This is really complex topic in face of i18n. I don’t think this should be solved in liquid core

On Sat, Aug 31, 2019 at 10:20 AM Mike Angell [email protected] wrote:

Proposed new addition for a number format filter. Resolves outstanding issues and really complex workarounds.

Usage

{{ price | format: decimal_places, thousands_separator, decimal_separator}}

Examples

{{ 20 | format: 2}} -> 20.00 {{ 2000000 | format }} -> 2,000,000.00 {{ 2000000 | format: 0, '' }} -> 2000000 {{ 2000000 | format: 2, '#', '@' }} -> 2#000#000@00

Resolves

#1087 https://github.com/Shopify/liquid/issues/1087 #567 https://github.com/Shopify/liquid/issues/567

@pushrax https://github.com/pushrax @ashmaroli https://github.com/ashmaroli @fw42 https://github.com/fw42 @Shopify/guardians-of-the-liquid https://github.com/orgs/Shopify/teams/guardians-of-the-liquid @Shopify/liquid https://github.com/orgs/Shopify/teams/liquid

You can view, comment on, or merge this pull request online at:

https://github.com/Shopify/liquid/pull/1146 Commit Summary

• Add new format filter

File Changes

• M lib/liquid/standardfilters.rb https://github.com/Shopify/liquid/pull/1146/files#diff-0 (6)
• M test/integration/standard_filter_test.rb https://github.com/Shopify/liquid/pull/1146/files#diff-1 (17)

Patch Links:

• https://github.com/Shopify/liquid/pull/1146.patch
• https://github.com/Shopify/liquid/pull/1146.diff

— You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub https://github.com/Shopify/liquid/pull/1146?email_source=notifications&email_token=AAAACWYZBFLU5VBJSZDQVMLQHJ427A5CNFSM4ISTZZCKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4HIS3LDA, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAACW2IX7A5LRTIPHAPHDLQHJ427ANCNFSM4ISTZZCA .

--

• tobi CEO Shopify

[tobi]

but yea, those tickets are pretty convincing that something is needed. In Shopify we should override this with a method that is i18n aware for the punctuation settings.

The implementation is pretty slow as it is. Not sure it matters.

[shopmike]

I don't think this is trying to solve i18n but just provides a convenience method for formatting a number if you wanted to do so which even I've come across a lot of times I've wanted to do this with Shopify for numbers that may be in a metafield etc. If you did really want to provide international support that can be done within a system with an i18n drop.

{{ 2000000 | format: i18n.precision, i18n.thousands, i18n.decimal }}

From a Shopify perspective this has been reported so many times, I know there is a money filter but I agree with most of these its about rounding units such as weight and other product data.

https://community.shopify.com/c/Shopify-Design/Two-decimal-places/td-p/130804 https://community.shopify.com/c/Shopify-Design/Always-show-2-decimal-places/m-p/299016 https://community.shopify.com/c/Shopify-Design/how-to-show-4-digits-after-decimal-in-shopify/m-p/361605 https://community.shopify.com/c/Shopify-Discussion/2-Decimal-Places-Requirement-e-g-5-50p-0-0550/m-p/423050 https://community.shopify.com/c/Shopify-Discussion/Decimal-places/m-p/33570

So use cases are

Weights -> 15.0kg vs 15.00kg Garment measurements -> 10.01cm x 5.4cm x 12.5cm -> 10.01cm x 5.40cm x 12.50cm Version numbers -> v1.0 -> v1.00 Total customers -> 1000000 -> 1,000,000 Thread count -> 1000 -> 1,000

• so many more

I can make the defaults more ISO focused which suggests a space for thousands rather than the comma

[shopmike]

In Shopify we should override this with a method that is i18n aware for the punctuation settings.

Agree the defaults can be smarter when implemented internally. I've just made a change so this is easier to do

[shopmike]

Other random stuff this can provide a simple solution for.

https://community.shopify.com/c/Shopify-Design/Proper-formatting-for-shop-phone/m-p/118316 https://community.shopify.com/c/Shopify-Design/Formatting-shop-phone-for-use-in-notifications/m-p/108698 https://community.shopify.com/c/Shopify-Design/Format-Phone-Number/m-p/110194

{% phone_number | divided_by: 10000 | format: 4, '-', '-' %}

[tobi]

Urg we should absolutely add | format_phone or equivalent to Shopify’s liquid if we expose shop.phone

On Sat, Aug 31, 2019 at 7:50 PM Mike Angell [email protected] wrote:

Other random stuff this can provide a simple solution for.

https://community.shopify.com/c/Shopify-Design/Proper-formatting-for-shop-phone/m-p/118316

https://community.shopify.com/c/Shopify-Design/Formatting-shop-phone-for-use-in-notifications/m-p/108698

https://community.shopify.com/c/Shopify-Design/Format-Phone-Number/m-p/110194

{% phone_number | divided_by: 10000 | format: 0, '-', '-' %}

— You are receiving this because you are on a team that was mentioned. Reply to this email directly, view it on GitHub https://github.com/Shopify/liquid/pull/1146?email_source=notifications&email_token=AAAACW5B5CDE3LJW2TM4763QHL7TBA5CNFSM4ISTZZCKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD5TXIBA#issuecomment-526873604, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAACWZO2DVATX75TOHJN2DQHL7TBANCNFSM4ISTZZCA .

--

• tobi CEO Shopify

[shopmike]

Rails has nice support for all of these including good i18n support. Maybe can just open these up in Shopify

[tobi]

the filters name should be format_number. If we really want to ship format filter then I think we should consider to ship a sprintf style filter in that case. Ruby comes with a really powerful implementation that supports precision but I'm a little bit worried about it's security. Not sure that it has been extensively fuzzed.

But I'm warming up to the idea. I think we should ship format that exposes sprintf and additional format_number with your signature that can use format internally. In Shopify I'd like to ship format_phone based on libphonenumber and override format_number with a version that uses locale aware punctuation by default. @Thibaut curious about your thoughts here.

[Thibaut]

I think https://github.com/Shopify/liquid/pull/1146#issuecomment-526869348 makes a convincing case of the need for a generic "number formatting" filter beyond the money use-case (solved already) and phone use-case (:100: to adding an i18n-aware format_phone filter in Shopify).

Generic filters carry the risk of being good enough to solve some use-cases without solving the entire problem domain, which then leads to a slow-roll of bad code that isn't quite painful enough for us to do something about, but still takes its toll.

format as suggested may fall into this trap in that it uses positional arguments and hardcodes three features, without leaving much room for solving other use-cases down the road (for example, controlling what the plus/minus sign looks like). At minimum I would use named arguments, so that we can easily extend the functionality in the future:

{{ number | format_number: precision: 2, delimiter: ",", separator: "." }}

Exposing Ruby's sprintf would likely solve the problem domain in full but beside the potential security vulnerability, I find the documentation hard to digest and the "format string" argument hard to read. In my mind it doesn't fit with Liquid's design philosophy, so I'd stick with format_number only (unless there's an open standard for number formatting, but I couldn't find one).

[shopmike]

I'm down for the named params, I was considering this as it is a similar approach used by the rails methods so they can handle all the domain specific functions which use a core function to do the overall modification.

Also I agree sprintf isn't intuitive to anyone and for me Liquid is about simplicity. I don't need to know how to program to understand something like number_format as its descriptive in everything it does. sprintf on the other hand comes back to needing to understand a spec, similar to opening up regex.

[shopmike]

Plus I don't know if ruby sprintf has it but running a class through it could be an easy way to expose all methods, some of which may not have been intended to be found and used.

[shopmike]

@Thibaut changes made

[tobi]

Should we test inputs as well? What does this filter do with strings of increasingly more mangled numbers?

[shopmike]

I purposely used the Util.to_number method thinking that it would cover this aspect of it. Double-checking though the Util class has no tests at all. I'll create an Issue for this and then put something together

[ffes]

Is there a change this PR will get some TLC any time soon? I would love to see this feature in a next release.

[bmandl]

Are there any updates on this one? This filter would be really usefull.