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

Make number coercion behave more like Ruby #473

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nbudin
Copy link

@nbudin nbudin commented Jul 3, 2022

  • Tests created for any new feature or regression tests for bugfixes.

liquid-rust doesn't quite behave like the Ruby implementation of Liquid when it comes to coercing other types into integers and floats.

There are three major differences I've found between Rust's behavior and Ruby's here, and I've tried to account for all of them in this PR:

  • Ruby's behavior is to return 0 when converting an unparsable string to an int, and return 0.0 when converting an unparsable string to a float. Example:
    irb(main):004:0> Liquid::Template.parse("{{ 'something' | plus: 0 }}").render
    => "0"
    irb(main):005:0> 'something'.to_i
    => 0
    
  • Ruby truncates floating point numbers when converting to integers.
  • Similarly, if asked to convert a string representation of a floating point number to an integer, Ruby will truncate it.

Fixing these behaviors does constitute a breaking change for liquid-rust, since in order to fix them, I had to change several existing tests. I'm also not really sure that my approach to parsing floating-point number strings as integers is the best one possible - it feels a little hacky and perhaps there's a better way.

@epage
Copy link
Member

epage commented Jul 11, 2022

Looks like this broke some other cases somehow.

@nbudin
Copy link
Author

nbudin commented Jul 11, 2022

Looks like this broke some other cases somehow.

Ah yeah, I see that it did. I'll look into those and see if I can figure out what's up.

…evious behavior and use them where we're assuming the strict behavior
@nbudin
Copy link
Author

nbudin commented Jul 11, 2022

@epage First off, sorry for breaking the tests! This is my first time working with a workspace project, and I didn't realize that simply running cargo test without --workspace would skip most of the tests.

I've worked through all the test failures. It appears that a bunch of logic internal to the stdlib as well as the derive macros was assuming that an unparsable integer would come back as None. In order to keep compatibility with this logic, I've introduced to_integer_strict and to_float_strict methods that preserve the prior behavior, and changed all the places that seemed to rely on it to use these instead. The tests are now passing.

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