search

datamapper / dm-types

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

Custom type migration bugfix #45

Open jpr5 opened 13 years ago

jpr5 commented 13 years ago

Simple enough use case:

Json property needs more than 64k, so adjust the :length the same way you do Text's. b00m on migration.

My solution was to add an additional dm-migrations' Adapter#type_map lookup for property.class.superclass, before falling back to the underlying DM primitive.

This will solve the vast majority of cases, like Json < Text. It doesn't solve for any cases of inheritance depth from builtin type > 1: MoreThanJson < Json. (MoreThanJson.class.superclass won't be in Adapter#type_map's table.) I argue that a practical 99% is better than a real 0%.

More detailed explanation in both the commits and the integration spec I provided for dm-types.

FYI merging this pull will require 2 more steps:

  1. merge the 1-line change on custom_type_migration branch over at datamapper/dm-migrations (https://github.com/datamapper/dm-migrations/pull/31)
  2. revert commit e13b1837, which was only necessary to show failing/passing specs across dm-types/dm-migrations
solnic commented 13 years ago

@jpr5 just for the record, this issue will be addressed in 1.3.0. I'm working on the improved property API which is already merged in to master in dm-core, dm-types and dm-migrations (see my recent commits in those repos)

jpr5 commented 13 years ago

@solnic That's great. Does that mean you're saying don't merge this pull request in the meantime? The fix itself is 1 line (in dm-migrations).

solnic commented 13 years ago

@jpr5 ok so as I commented on dm-migrations' pull request it's already merged in. I didn't merge this pull request though as the specs you added fail for me. Let's work this out for 1.2.1