datamapper / dm-types

DataMapper plugin providing extra data types
http://datamapper.org/
MIT License
55 stars 80 forks source link

Add IPAddressInteger Property: IPv4/v6 => Integer #38

Closed d11wtq closed 13 years ago

d11wtq commented 13 years ago

We store our IP Addresses as integers in the database. We thought you might want to include it in dm-types, for completeness.

postmodern commented 13 years ago

I provided the same property in dm-types-legacy.

@dkubb Is this property becoming popular enough for inclusion into dm-types?

d11wtq commented 13 years ago

Yours looks better than mine actually. Thanks for linking to that. I didn't fully understand what the difference between load/typecast and dump/typecast_to_primitive were for. I think it makes sense after looking at your version. typecast handles the fact that users may assign various data types to the property, while dump just converts from the canonical data type to the underlying primitive form.

If either version goes into dm-types, just use @postmodern's. It seems more sensible to store a 4-byte integer in the database instead of a 15-byte string, both for storage reasons and for index performance.

solnic commented 13 years ago

@d11wtq the difference between #load and #typecast is that the former is called when a value is retrieved from the datastore and the latter is called when a value comes from the user input. It may happen that both methods do exactly the same thing so there are properties that, for instance, call #load inside #typecast. That's why it may be confusing.

Currently custom properties are not well integrated with dm-core to be honest. There are still some limitations that we need to deal with. For example DM assumes primitive can be only set to one class when in reality it could be more than one (like in JSON property). This is the reason why sometimes you need to override #valid? and/or #primitive? to get your property working correctly.

Another problem is that DO adapters reflect property class hierarchy on the database field types. So if you want to store something as a string you need to inherit from String property or explicitly set primitive to String. This may sound correct but it's not because there is a line between how you want to store something and how it should look like in Ruby land. You may want to have a time property but store it as a string - right now you'd have to inherit from String and override primitive setting and include time-related typecast module when in the perfect world you should simply inherit from Time property and use a different setting to tell DO adapters that you will store it as a String.

For further read you can take a look at my Property 2.0 draft (which actually may become 1.2 heh): https://gist.github.com/784350

ps. I'm closing this pull request cause we want to keep dm-types rather small and limit it to the most popular use cases. If you think we should improve IPAddress property then it's cool and we should definitely do it when we bump to 2.0. For now let's use @postmodern version :)

d11wtq commented 13 years ago

Hmm, my version wasn't behaving well until the object has been saved:

resource = AResource.new
resource.ip = "127.0.0.1"
# => 2130706433 # Whoops!

So now I'm using your version @postmodern, but the object is never valid:

resource = AResource.new
resource.ip = "127.0.0.1"
# => #<IPAddr: IPv4:127.0.0.1/255.255.255.255> # OK!

resource.valid?
# => false

resource.errors.full_messages
# => ["Ip must be an integer"]

I can't figure out what combination of typecast and/or typecast_to_primitive is needed.

d11wtq commented 13 years ago

It works if I define

def custom?
  true
end

But then it considers the object as valid, regardless of the datatype given to it. For now, I'll have to run with this, but if anyone knows how to fix the validation problem so it actually validates the datatype, please let me know.

solnic commented 13 years ago

@d11wtq this is exactly one of the reasons why I want to do the change described in 'property 2.0 draft' gist

d11wtq commented 13 years ago

Anything that makes using DM easier is a good thing! Do it :)