Improved coercion: primitives and error handling

[maxlinc]

This includes #199 (coerce only when necessary) but also fixes #161 (Boolean coercion?)

Actually, it adds:

• Integer coercion (via to_i)
• Float coercion (via to_f)
• String coercion (via to_s)
• Symbol coercion (via to_sym)
• Boolean type-checking, using the pseudo-type :boolean
• Better coercion error messages

Boolean type checking is done via:

coerce_key :foo, :boolean

This will check that foo is a boolean. It would be possible to coerce foo to a boolean, but there isn't a standard convention so we'd need to make a choice. For example, integers can be converted to a boolean as:

• non-zero is true, 0 is false (!i.zero?)
• positive is true, otherwise false (i > 0)
• any value is true, nil is false (!!i)

This PR also adds better error messages for when coercion fails. It will indicate if a type is not coercible:

subject.coerce_key :foo, TrueClass
instance[:foo] = true
#=> TypeError: Cannot coerce property :foo from TrueClass to TrueClass: TrueClass is not a coercable type

Or if a type is coercable under some circumstances, but not from that specific value:

subject.coerce_value Fixnum, Symbol
instance[:foo] = 1
#=> TypeError: Cannot coerce property :hi from Fixnum to Symbol: undefined method `to_sym' for 1:Fixnum

[maxlinc]

@dblock one question and a couple notes

Notes

I created a new PR rather than updating #199 because that PR just covers lazy coercion and has fewer open questions. This covers several coercion corner cases, but has some open questions and surprising behavior to review. So I left #199 for now, but if it looks like this will be merged #199 can be closed.

This might fix #115 but I need a test case to confirm.

Question

Since there are many ways to coerce to a boolean, what if hashie accepted a proc directly?

Rather than:

coerce_value Integer, :boolean

What about:

coerce_value Integer, ->(v) { !v.zero? }
# or coerce_value Integer, ->(v) { v > 0 }
# or coerce_value Integer, ->(v) { !!v }

[dblock]

+1 on the Proc idea, however don't feel too compelled to do this at the same time.

There's a failure on Rubinius that doesn't look like a fluke, lets at least understand it?

[dblock]

Can this be un-pendingfied?

[maxlinc]

I left it there to catch you attention :)

The specs for coerce_value are:

• 'coerces any value of the exact right class'
• 'does not coerce superclasses'

So this does not work:

subject.coerce_value Integer, String

But you can do this:

subject.coerce_value Fixnum, String
subject.coerce_value Bignum, String

Is that an acceptable limitation or would you prefer to handle Integer and Numeric? I suppose core types are already a special case, so maybe the spec can be 'does not coerce superclasses (except for core numeric types)'

[dblock]

I think most people are going to be using Integer and Numeric, so I think we should handle those, even explicitly. Down with what you propose with the spec (does not coerce superclasses except for numeric types).

[dblock]

This needs a CHANGELOG entry, too, please, and possibly an UPGRADING note.

[maxlinc]

Updates:

• Added support for Complex and Rational. Works fine on Ruby 1.9.3+, if you still pseudo-support older versions might want to add require 'complex' and require 'rational' statements
• I added support for boolean via a proc, rather than :boolean.
• Fixed the rbx test issue. It was because of a difference with TrueClass.new. I created a "NotInitializable" class for the test instead.
• I noticed that nil and false are never coerced, because of unless value && into. This probably makes sense, even though they're both often coercible in theory. I left it as pending for now to confirm this is true on all rubies (rspec 3 tests that pending cases fail).

FYI - Pending tests do run (and must fail) on rspec 3, so the pending tests check the behavior is not working on any platform.

@dblock Can you review the changes so far and decide what the behavior should be for the pending scenarios? Once that's done I'll work on the final updates and documentation.

[dblock]

This is excellent work. LMK if my comment is not enough. As always, I welcome a more opinionated thought on this from your side because you've had your hands in the coercion for the last few - if anyone is best qualified to make a call is yourself.

[maxlinc]

Yeah, your comments were enough. I'll target the pareto principal: the 20% of coercions that will be used 80% of the time. Right now I'm probably covering 80% - but the 80% that are easiest to implement rather than most used.

It would be good to test this on some real world projects that have deeply nested coercible objects before saying its ready to merge. Do you know any real-world projects that are good candidates for regression testing coercion?

[maxlinc]

This might be done! The tests cover all the behavior I think makes sense. The error messages when coercion fails are definitely better, but it might be possible to improve them further.

I won't be touching this next week because I'm on vacation, but if you can review and test this PR on some real-world projects it'd be helpful. There are significant changes here - not just coercion types that were previously not coercible, but it also now avoids unnecessary coercion (as discussed earlier), coerces false (but still not nil), and raises different errors.

Here's a diff of the rspec documentation for your convenience:

@@ -2,6 +2,7 @@
   #coerce_key
     should be respond to :coerce_key
     runs through coerce on a specified key
+    skips unnecessary coercions
     supports an array of keys
     supports coercion for Array
     supports coercion for Set
@@ -10,6 +11,31 @@
     supports coercion for Hash with primitive as value
     calls #new if no coerce method is available
     coerces when the merge initializer is used
+    nesting
+      coeces nested objects
+      coeces nested arrays
+      coeces nested hashes
+    coercing core types
+      coerces from String to Integer via to_i
+      coerces from Integer to Integer via to_i
+      coerces from Rational to Integer via to_i
+      coerces from String to Float via to_f
+      coerces from Integer to Float via to_f
+      coerces from Rational to Float via to_f
+      coerces from String to String via to_s
+      coerces from Integer to String via to_s
+      coerces from Rational to String via to_s
+      coerces from String to String via to_s
+      coerces from Symbol to String via to_s
+      coerces from String to Symbol via to_sym
+      coerces from Symbol to Symbol via to_sym
+      can coerce String to Rational when possible
+      can coerce String to Complex when possible
+      coerces collections with core types
+      can coerce booleans via a proc
+      raises errors for non-coercable types
+      can coerce false
+      does not coerce nil
     when #replace is used
       coerces relevant keys
       sets correct values
@@ -29,3 +55,9 @@
       coerces any value of the exact right class
       coerces values from a #replace call
       does not coerce superclasses
+    core types
+      coerces String to Integer when possible
+      coerces non-numeric from String to Integer
+      raises a CoercionError when coercion is not possible
+      coerces Integer to String
+      coerces Numeric to String

[dblock]

I think this can be merged, it needs a CHANGELOG/README/UPGRADING updates.

[dblock]

Bump @maxlinc, would really like this to go in.

[maxlinc]

Done.

I'm on vacation this week, so if only minor edits are needed feel free to take what I have and refactor or rephrase whatever you want. Otherwise I'll respond to any questions next week.

Also, I was hoping to get it to track the "chain" for deeper coercion errors, but it looks like something that may need to wait for a future PR.

It would be something like this:

        expect do
          instance[:nested_list] = [
            { foo: 123, bar: '456' },
            { foo: 234, bar: '567' },
            { foo: 345, bar: :xyz }
          ]
        end.to raise_error(Hashie::CoercionError, /Cannot coerce property nested_list[3].bar/)

Right now it just says "Cannot coerce property :bar" so tracking down the exact class and/or object that failed can be tricky. I wanted to push the current key and/or array index to a stack during coercion, so at the end you have [:nested_list, 3, :bar], but I don't see a way to do that with refactoring coercion.rb.

[dblock]

Perfect, merging as is. Thanks a lot.