Skip to content

Property Improved #139

Merged
merged 12 commits into from Sep 12, 2011

3 participants

@solnic
DataMapper member
solnic commented Sep 11, 2011

Hey!

This is still a WIP but I'm opening a pull request early so we can be discussing changes while I'm finishing this stuff.

This branch makes two relatively small but significant changes. First it adds dump_as/load_as property options as described here: https://gist.github.com/784350; it also removes typecast logic from dm-core and replaces it with Virtus Coercion system.

Let me know what you think :)

@dkubb dkubb commented on the diff Sep 12, 2011
lib/dm-core/property.rb
- @primitive = self.class.primitive
- @field = @options[:field].freeze unless @options[:field].nil?
- @default = @options[:default]
+ @field = @options[:field].freeze unless @options[:field].nil?
@dkubb
DataMapper member
dkubb added a note Sep 12, 2011

I realize the original code did this, but this seems to be freezing the object that was provided to the method, even if it is a hash value.

In general we try not to mutate any objects passed into methods, unless that is the method's primary purpose (which is rare).

@dkubb
DataMapper member
dkubb added a note Sep 12, 2011

The safest approach for freezing objects can be seen in veritas: https://github.com/dkubb/veritas/blob/master/lib/veritas/support/immutable.rb#L81-116

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dkubb dkubb commented on an outdated diff Sep 12, 2011
lib/dm-core/property/class.rb
- primitive ::Class
-
- # Typecast a value to a Class
- #
- # @param [#to_s] value
- # value to typecast
- #
- # @return [Class]
- # Class constructed from value
- #
- # @api private
- def typecast_to_primitive(value)
+ # @api semipublic
+ def typecast(value)
+ return unless value
@dkubb
DataMapper member
dkubb added a note Sep 12, 2011

This should probably check value.nil? explicitly here instead. If false was passed in we just want to pass it through, not silently transform it into nil.

@dkubb
DataMapper member
dkubb added a note Sep 12, 2011

Come to think of it,. are there other places in dm-core or dm-types where we just do return unless value rather than return unless value.nil? and thus potentially coerce false to nil when we should be passing it through?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dkubb dkubb commented on the diff Sep 12, 2011
lib/dm-core/property/numeric.rb
def initialize(model, name, options = {})
super
- if @primitive == BigDecimal || @primitive == ::Float
+ if kind_of?(Decimal) || kind_of?(Float)
@dkubb
DataMapper member
dkubb added a note Sep 12, 2011

Later on we should consider extracting this block into a module that is mixed into Decimal and Float properties.

I don't like the idea of a class having any direct knowledge of their subclases.

@solnic
DataMapper member
solnic added a note Sep 12, 2011

Yes, I never liked it. Those weird bits of code are 'left-overs' from the DM::Type era. To be honest with the DM 2.0 on the horizon my motivation to clean this up is getting smaller and smaller each day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dkubb dkubb and 1 other commented on an outdated diff Sep 12, 2011
lib/dm-core/property/object.rb
# @api semipublic
def dump(value)
+ if self.class == Object
@dkubb
DataMapper member
dkubb added a note Sep 12, 2011

I would generally recommend using Object.equal?(self.class). Not only is it more efficient, it's also almost guaranteed that Object#equal? won't be overridden -- of course it's highly unlikely that self.class#== won't be overridden too, but not quite as much.

@solnic
DataMapper member
solnic added a note Sep 12, 2011

Actually I ended up using instance_of?(Object) - what do you think?

@dkubb
DataMapper member
dkubb added a note Sep 12, 2011

Oh, that's even better than what I was suggesting. For some reason I was thinking you had a class that you were comparing to Object, but since you have an instance #instance_of? is the most intention revealing approach.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dkubb dkubb commented on the diff Sep 12, 2011
spec/public/property/binary_spec.rb
@@ -4,7 +4,7 @@ describe DataMapper::Property::Binary do
before :all do
@name = :title
@type = described_class
- @primitive = String
+ @load_as = String
@dkubb
DataMapper member
dkubb added a note Sep 12, 2011

It looks like there are small formatting fixes needed in the specs for what (I assume) was changed via find/replace.

@solnic
DataMapper member
solnic added a note Sep 12, 2011

blame sed for this awful mistake ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dkubb
DataMapper member
dkubb commented Sep 12, 2011

@solnic this is a pretty amazing reduction in code. Do the dm-types specs pass with this change?

Most of the comments I've left are really minor in nature, I really think this is a pretty awesome change. I love removing code from DM and replacing it with library code that's better tested. It's also a nice way to get virtus used more now.

@solnic solnic merged commit a42c0ff into datamapper:master Sep 12, 2011
@dkubb dkubb commented on the diff Sep 12, 2011
lib/dm-core/associations/many_to_one.rb
@@ -266,7 +266,7 @@ module DataMapper
:unique => @unique
)
- if target_property.primitive == Integer
+ if target_property.instance_of?(Property::Integer)
@dkubb
DataMapper member
dkubb added a note Sep 12, 2011

I think this purpose of this was to catch any property subclasses with Integer primitives, not necessarily only Property::Integer classes.

However, for other parts of DM to work, people would probably have to subclass Property::Integer anyway if the they needed to store an Integer, so maybe this should use #kind_of? ? What do you think @solnic?

@solnic
DataMapper member
solnic added a note Sep 12, 2011

@dkubb yes in other gems (dm-validations for example) I used #kind_of? cause there we may deal with custom subclasses; notice that this code here deals with FKs which currently are always created as Integer properties. For this to fail somebody would have to manually create an fk using an integer sub-class. Maybe "just in case" we should use #kind_of? here too. I don't have a strong opinion to be honest.

@dkubb
DataMapper member
dkubb added a note Sep 12, 2011

I tend to use the strongest match that works, and then fallback to something less specific or duck typing if I have a valid use case.

In this case I think we have a valid use case though, and it's probably best handled by duck typing rather than asserting the class. I would probably change this to if target_property.respond_to?(:min) && target_property.respond_to?(:max), which will pass-through the min/max if the property type supports it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@emmanuel
DataMapper member
@solnic
DataMapper member
solnic commented Sep 13, 2011

@emmanuel thanks man, it was fun :)

I'm not sure how to tackle the virtus transition to be honest. I still need to think about possible solutions. @dkubb suggested that I could turn Property into a sub-class of Virtus::Attribute to ease the transition. That seems like a good idea.

What do you mean by 'has-a Virtus::Attribute'? Effectively we want to rip out Property and have it replaced by Virtus.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.