search

t6d / smart_properties

Ruby accessors on steroids
MIT License
177 stars 20 forks source link

Use equality test for nil as a last resort #64

Closed lreeves closed 6 years ago

lreeves commented 6 years ago

I ran into an issue when using properties that are ActiveRecord::Relation objects - essentially what was happening was extra queries being made to the database during smart property validation. It turns out that ActiveRecord (and probably other things) can have side effects when using the built-in equality methods.

This PR changes the order of the nil-checking for properties and uses the #nil? method as the default now; if a NoMethodError is raised, then we can safely do a nil singleton equality check since the object being validated is most likely descending from BasicObject.

t6d commented 6 years ago

Have you tried nil == object alternatively? This would invoke NilClass#== rather than invoking #== on the object.

lreeves commented 6 years ago

That would work too, I went this way since it's already rescuing from NoMethodError; want me to change it?

t6d commented 6 years ago

If I introduced the line only for optimization purposes then the change would make sense, otherwise, we can take it out altogether. NilClass#nil? will always be true. If I introduced the check against nil for any other reason, there is likely a test that is going to fail. Maybe the best option is to actually take out the line and just add the test.

lreeves commented 6 years ago

Done! Want a version bump as well or should I leave that to you?