Update item_property to recognize integers

[summerisgone]

This is a 🐛 bug fix

• I've added tests

• no need to update documentation

• The test suite passes locally

Summary

Method item_property aggressively parses floats from values. In order to properly render integers I added integer check before float check. I guess better solution would be type check, but I'm afraid it might break other things.

Context

Fixes #7827

--
P.S.
Thanks for wonderful software, glad to bring my 5 cents!

[ashmaroli]

Couple of recommendations:

• The two regexes would best be defined as a pair of private_constant instead.
• Prefer stashing the stringified property in a local variable instead of opening chances for multiple stringifications (e.g. when the first branch is going to evaluate to a false)
• Prefer using String#match? over String#=~ when the logic doesn't involve reading the matchdata.

Concerns addressed..

[parkr]

Two quick questions:

1. Does the Liquid library have a method for casting to numbers that we could use instead of our own implementation?
2. Are the Windows failures expected?

[ashmaroli]

2. Are the Windows failures expected?

Windows failures are only on GH Actions: There's a PR to patch that: #7885

[ashmaroli]

1. Does the Liquid library have a method for casting to numbers that we could use instead of our own implementation?

Liquid has a utility function:
https://github.com/Shopify/liquid/blob/57c9cf64ebc777fe5e92d4408d31a911f087eeb4/lib/liquid/utils.rb#L48-L63

    def self.to_number(obj)
      case obj
      when Float
        BigDecimal(obj.to_s)
      when Numeric
        obj
      when String
        /\A-?\d+\.\d+\z/.match?(obj.strip) ? BigDecimal(obj) : obj.to_i
      else
        if obj.respond_to?(:to_number)
          obj.to_number
        else
          0
        end
      end
    end

[summerisgone]

@parkr @ashmaroli

Does the Liquid library have a method for casting to numbers that we could use instead of our own implementation?

Actually it is not required to cast to string or to any type at all, since data comes in typed manner from yaml files. But item_property function is used for filtering, sorting and grouping. I think in-place solution might fit better then moving cast to templates, since it would have less side effects.

[ashmaroli]

it is not required to cast to string or to any type at all

@summerisgone The reason #parse_sort_input exists is to allow sorting of stringified integers.
For example, Ruby would sort ["15", "20", "10", "120"] into ["10", "120", "15", "20"] which is not acceptable. But if we were to cast those strings to Integer type and then sort the array, the result would be [10, 15, 20, 120], which would in turn result in the expected results for filtering, sorting or grouping.

[summerisgone]

@ashmaroli what is your suggestion, to import to_number from Liquid utils instead of parse_sort_input? Shall I update the PR?
Consider it won't parse spaces, but I think it might be OK 🤷‍♂️

[ashmaroli]

@summerisgone Its a public method. So you can call it directly. For example:

Liquid::Utils.to_number(input)

However, you may need to add additional tests to ensure expected results for various objects.

[summerisgone]

@ashmaroli I tried to use Liquid::Utils.to_number, but it does convert all non-numbers to 0, which is not desired when sorting strings. I think it is not drop-in replacement for parse_sort_input unfortunately

[summerisgone]

@ashmaroli what else blocks the merge?

[ashmaroli]

@summerisgone The change looks okay.
I'm just wondering if there's a way to go about this without stringifying property..
But since the diff shows that property is already being stringified on master, you can rest assured that this will get merged.
Optimizations if any will be via a separate pull request.

[mattr-]

@jekyllbot: merge +minor