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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add max and min filters #954

Merged
merged 1 commit into from
Nov 30, 2017
Merged

Add max and min filters #954

merged 1 commit into from
Nov 30, 2017

Conversation

nithinbekal
Copy link
Contributor

This will introduce max and min filters. An example use case:

{%- assign width = 250 -%}

{%- if image.width < width -%}
  {%- assign width = image.width -%}
{%- endif -%}

The above can be replaced with:

{%- assign width = image.width | max: 250 -%}

If invalid inputs are given (eg. "foo"), they would default to 0 (eg. {{ "foo" | max: 5 } returns 0, and {{ "foo" | min:5 }} returns 5), which is consistent with other filters that work on numbers (ceil, floor etc), but I haven't added test cases for those.

@Thibaut

@nithinbekal nithinbekal requested a review from Thibaut November 28, 2017 00:00
@@ -355,6 +355,16 @@ def floor(input)
raise Liquid::FloatDomainError, e.message
end

def max(input, n)
result = [input, n].map { |number| Utils.to_number(number) }.min
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Allocating an array just for this is not great, we're not running on something fancy like TruffleRuby that can optimize that allocation away.

@nithinbekal
Copy link
Contributor Author

@pushrax Updated the methods to use conditions instead of allocating arrays, and added additional tests for number-like things.


assert_template_result "4.5", "{{ 4.5 | max:5 }}"
assert_template_result "5", "{{ width | max:5 }}", 'width' => NumberLikeThing.new(6)
assert_template_result "4", "{{ width | max:5 }}", 'width' => NumberLikeThing.new(4)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could add a test for {{ 5 | max: width }}

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@nithinbekal
Copy link
Contributor Author

Added tests for {{ 5 | max: width }}, as suggested by Thibaut.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants