DictionaryValue should return the item count for ToNumberValue()

[deanebarker]

DictionaryValue.ToNumberValue() always returns 0.

https://github.com/sebastienros/fluid/blob/main/Fluid/Values/DictionaryValue.cs#L91

The underlying value is IFluidIndexable which has a Count property. It should return this from ToNumberValue(), to match the behavior of ArrayValue.

[hishamco]

DictionaryValue.ToNumberValue() always returns 0.

It's right, because Dictionary can't be converted to number, similarlly you will find ToStringValue() returns empty string

[deanebarker]

If ArrayValue can convert to a number, why shouldn't DictionaryValue? They are both collections of items, one just happens to have keys.

[hishamco]

I disagree for ArrayValue too, @sebastienros I'm not sure why we should have array length

[hishamco]

For ArrayValue we could use the first element value, in case if the FluidValue is numeric

[sebastienros]

In which context do you use this? I am trying to find the behavior in Shopify, but can't repro so far. I know I can trigger it in Fluid with {{ 1 | plus: my_array }}. In Shopify this just return NaN. Though both will work with {{ 1 | plus: my_array.size }} or {{ 1 | plus: my_array.length }}

[sebastienros]

Also about ToStringValue() returning empty, this might be a bug actually. When using {{ _my_array | append: 'foo' }} it returns the same thing as {{ my_array }}foo so probably ToStringValue() should return the same thing as {{ my_dictionary }} or vice versa.

[deanebarker]

I guess was just confused by the discrepancy. Why does ArrayValue return a number and DictionaryValue does not? Did we just reproduce some Shopify weirdness?

[sebastienros]

If it's just the discrepancy, we might just want to fix the array one. Locally it doesn't break any tests which means there might have been no reason to return this number. However something like {{ my_array | plus: 1 }} return NaN in Shopify and LiquidJS se I wonder if we should do this or just assume 0 + 1 in this case as it's doing today.

[hishamco]

IMHO ToNumber(), ToString() should trying to convert the current value to their equivalent, no need to assume something that are not expected for the audience