Skip to content

Conversation

@yozna-kabra-sarda
Copy link

In If condition, while comparing floating numbers, comparison was failing. Fixed the issue.

@yozna-kabra-sarda
Copy link
Author

The fix given here will break the string comparisons which is not correct. So closing the PR.

@yozna-kabra-sarda
Copy link
Author

I will give better fix for integers, floating numbers and string comparisons

@sanko
Copy link
Owner

sanko commented Sep 15, 2022

You're gonna need to include tests.

@yozna-kabra-sarda
Copy link
Author

I have added the test cases too. Please do the needful.

1 similar comment
@yozna-kabra-sarda
Copy link
Author

I have added the test cases too. Please do the needful.

@yozna-kabra-sarda
Copy link
Author

I have fixed the test case which was throwing error. Please look into this and do the needful.

@sanko
Copy link
Owner

sanko commented Sep 19, 2022

Don't rewrite tests to fit the bug you introduced. By turning a regex designed to match nonnumeric data into one that matches floats, you broke proper variable parsing. The test was correct and used the dot notation to get string size as documented here: https://shopify.github.io/liquid/filters/size/.

Btw, I could do without the 40+ emails from you and your colleagues over the last 7 days; I do not work for Exceleron.

@yozna-kabra-sarda
Copy link
Author

Apologies for the inconvenience caused but we are just trying to fix the actual issue to improve the module.

We found one more usecase that was failing. So we have given the fix for that too.

Usecase :
In the below mentioned example, we are checking if 10 is less than 10, which is actually not, but still it is executing it to true.

use Template::Liquid;
my %data = (balance => 10);

my $input = '
  {% if balance < 10 %}
      Got Balance {{ balance }}  is less  !!
  {% else %}
      Balance {{ balance }} is more
  {% endif %}
';

my $tt1 = Template::Liquid->parse($input);
print $tt1->render(%data) or die $tt1->error;
print "\n\n"; 

Output => Got Balance 10 is less !!

@yozna-kabra-sarda
Copy link
Author

yozna-kabra-sarda commented Oct 11, 2022 via email

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.

2 participants