search

hassox / dm-zone-types

Time zone aware data types
13 stars 2 forks source link

all(:time.not => nil) is broken #5

Closed mltsy closed 12 years ago

mltsy commented 12 years ago

This works using DateTime as a type, but not when I switch it to ZonedTime, in which case it always returns an empty collection:

class Thing
  include DataMapper::Resource

  property :id
  property :time, DateTime
end

Thing.all(:time.not => nil)
mltsy commented 12 years ago

It looks like it's actually the valid? method that's causing the error. I don't know why the valid? method is overridden... I haven't seen any documentation regarding overriding the valid? method. You could check the type and return an error in the dump method, right? Removing valid? seems to work fine and fix the issue.

If you don't want to remove it, I think the actual problem is that the "negate" parameter is being used wrong. Which is hard to tell since there's no documentation for it. But in the super method, it simply returns true when it would otherwise return false. In this one, it does that AND returns false when it would return true.

So, I think I will submit a patch to change that functionality... but I would consider removing the valid? method altogether, because there's some functionality in the super method (regarding :required, for instance) that is being lost in the overriding version, and I assume that will cause other issues as well (for required fields, for instance).

mltsy commented 12 years ago

Pull Request: https://github.com/hassox/dm-zone-types/pull/6