Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update item_property to recognize integers #7878

Merged
merged 4 commits into from Dec 10, 2019

Conversation

@summerisgone
Copy link
Contributor

summerisgone commented Oct 26, 2019

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!

Copy link
Member

ashmaroli left a comment

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.
summerisgone and others added 2 commits Oct 31, 2019
@ashmaroli ashmaroli dismissed their stale review Nov 1, 2019

Concerns addressed..

@ashmaroli ashmaroli requested a review from jekyll/build Nov 1, 2019
@parkr

This comment has been minimized.

Copy link
Member

parkr commented Nov 4, 2019

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

This comment has been minimized.

Copy link
Member

ashmaroli commented Nov 4, 2019

  1. Are the Windows failures expected?

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

@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Nov 4, 2019

  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

This comment has been minimized.

Copy link
Contributor Author

summerisgone commented Nov 6, 2019

@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

This comment has been minimized.

Copy link
Member

ashmaroli commented Nov 6, 2019

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

This comment has been minimized.

Copy link
Contributor Author

summerisgone commented Nov 6, 2019

@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

This comment has been minimized.

Copy link
Member

ashmaroli commented Nov 6, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

summerisgone commented Nov 6, 2019

@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

This comment has been minimized.

Copy link
Contributor Author

summerisgone commented Dec 10, 2019

@ashmaroli what else blocks the merge?

@ashmaroli

This comment has been minimized.

Copy link
Member

ashmaroli commented Dec 10, 2019

@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-
mattr- approved these changes Dec 10, 2019
@mattr-

This comment has been minimized.

Copy link
Member

mattr- commented Dec 10, 2019

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit eb81dc0 into jekyll:master Dec 10, 2019
7 checks passed
7 checks passed
SUITE: test / OS: ubuntu-latest
Details
SUITE: test / OS: windows-latest
Details
SUITE: default-site / OS: ubuntu-latest
Details
SUITE: default-site / OS: windows-latest
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/jekyllrb/deploy-preview Deploy preview ready!
Details
jekyllbot added a commit that referenced this pull request Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can鈥檛 perform that action at this time.