This repository has been archived by the owner on Apr 17, 2018. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 154
Property Improved #139
Merged
Merged
Property Improved #139
Changes from all commits
Commits
Show all changes
12 commits
Select commit
Hold shift + click to select a range
9943ae4
require virtus
solnic 92d0588
Replace "primitive" with "dump_as" and add a new option "load_as" (WIP)
solnic ea82393
Integrate with Virtus Coercion system (WIP)
solnic d5d13b3
Don't coerce to string if the value is nil
solnic d165e7c
Use instance_of? in Object property load/dump methods
solnic 0c6525c
Regenerated gemspec
solnic bb30276
Turn on deprecation warnings
solnic 254102b
Check property instance type instead of primitive
solnic b407d57
Add Boolean#value_loaded? so it works wit the primitive validator
solnic 4999726
Don't typecast to const if the value is nil
solnic f3b5361
Set dump_as explicitly on all properties
solnic 973067f
Add load_class/dump_class aliases
solnic File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -300,24 +300,6 @@ module DataMapper | |
# * You may declare a Property with the data-type of <tt>Class</tt>. | ||
# see SingleTableInheritance for more on how to use <tt>Class</tt> columns. | ||
class Property | ||
module PassThroughLoadDump | ||
# @api semipublic | ||
def load(value) | ||
typecast(value) unless value.nil? | ||
end | ||
|
||
# Stub instance method for dumping | ||
# | ||
# @param value [Object, nil] value to dump | ||
# | ||
# @return [Object] Dumped object | ||
# | ||
# @api semipublic | ||
def dump(value) | ||
value | ||
end | ||
end | ||
|
||
include DataMapper::Assertions | ||
include Subject | ||
extend Equalizer | ||
|
@@ -337,6 +319,7 @@ def dump(value) | |
].to_set.freeze | ||
|
||
OPTIONS = [ | ||
:load_as, :dump_as, :coercion_method, | ||
:accessor, :reader, :writer, | ||
:lazy, :default, :key, :field, | ||
:index, :unique_index, | ||
|
@@ -352,10 +335,14 @@ def dump(value) | |
Query::OPTIONS.to_a | ||
).map { |name| name.to_s } | ||
|
||
attr_reader :primitive, :model, :name, :instance_variable_name, | ||
attr_reader :load_as, :dump_as, :coercion_method, | ||
:model, :name, :instance_variable_name, | ||
:reader_visibility, :writer_visibility, :options, | ||
:default, :repository_name, :allow_nil, :allow_blank, :required | ||
|
||
alias_method :load_class, :load_as | ||
alias_method :dump_class, :dump_as | ||
|
||
class << self | ||
extend Deprecate | ||
|
||
|
@@ -460,9 +447,15 @@ def options | |
end | ||
options | ||
end | ||
|
||
# @api deprecated | ||
def primitive(*args) | ||
warn "DataMapper::Property.primitive is deprecated, use .load_as instead (#{caller.first})" | ||
load_as(*args) | ||
end | ||
end | ||
|
||
accept_options :primitive, *Property::OPTIONS | ||
accept_options *Property::OPTIONS | ||
|
||
# A hook to allow properties to extend or modify the model it's bound to. | ||
# Implementations are not supposed to modify the state of the property | ||
|
@@ -680,11 +673,7 @@ def properties | |
|
||
# @api semipublic | ||
def typecast(value) | ||
if value.nil? || primitive?(value) | ||
value | ||
elsif respond_to?(:typecast_to_primitive) | ||
typecast_to_primitive(value) | ||
end | ||
Virtus::Coercion[value.class].send(coercion_method, value) | ||
end | ||
|
||
# Test the value to see if it is a valid value for this Property | ||
|
@@ -702,7 +691,7 @@ def valid?(value, negated = false) | |
if required? && dumped_value.nil? | ||
negated || false | ||
else | ||
primitive?(dumped_value) || (dumped_value.nil? && (allow_nil? || negated)) | ||
value_dumped?(dumped_value) || (dumped_value.nil? && (allow_nil? || negated)) | ||
end | ||
end | ||
|
||
|
@@ -726,7 +715,23 @@ def inspect | |
# | ||
# @api semipublic | ||
def primitive?(value) | ||
value.kind_of?(primitive) | ||
warn "#primitive? is deprecated, use #value_dumped? instead (#{caller.first})" | ||
value_dumped?(value) | ||
end | ||
|
||
def primitive | ||
warn "#primitive is deprecated, use #dump_as instead (#{caller.first})" | ||
dump_as | ||
end | ||
|
||
# @api semipublic | ||
def value_dumped?(value) | ||
value.kind_of?(dump_as) | ||
end | ||
|
||
# @api semipublic | ||
def value_loaded?(value) | ||
value.kind_of?(load_as) | ||
end | ||
|
||
protected | ||
|
@@ -749,10 +754,13 @@ def initialize(model, name, options = {}) | |
@name = name.to_s.chomp('?').to_sym | ||
@options = predefined_options.merge(options).freeze | ||
@instance_variable_name = "@#{@name}".freeze | ||
@coercion_method = @options.fetch(:coercion_method) | ||
|
||
@load_as = self.class.load_as | ||
@dump_as = self.class.dump_as | ||
|
||
@primitive = self.class.primitive | ||
@field = @options[:field].freeze unless @options[:field].nil? | ||
@default = @options[:default] | ||
@field = @options[:field].freeze unless @options[:field].nil? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
@default = @options[:default] | ||
|
||
@serial = @options.fetch(:serial, false) | ||
@key = @options.fetch(:key, @serial) | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
module DataMapper | ||
class Property | ||
class Binary < String | ||
include PassThroughLoadDump | ||
|
||
end # class Binary | ||
end # class Property | ||
end # module DataMapper |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,31 +1,20 @@ | ||
module DataMapper | ||
class Property | ||
class Boolean < Object | ||
include PassThroughLoadDump | ||
load_as ::TrueClass | ||
dump_as ::TrueClass | ||
coercion_method :to_boolean | ||
|
||
primitive ::TrueClass | ||
|
||
TRUE_VALUES = [ 1, '1', 't', 'T', 'true', 'TRUE' ].freeze | ||
FALSE_VALUES = [ 0, '0', 'f', 'F', 'false', 'FALSE' ].freeze | ||
BOOLEAN_MAP = Hash[ | ||
TRUE_VALUES.product([ true ]) + FALSE_VALUES.product([ false ]) ].freeze | ||
# @api semipublic | ||
def value_dumped?(value) | ||
value_loaded?(value) | ||
end | ||
|
||
def primitive?(value) | ||
# @api semipublic | ||
def value_loaded?(value) | ||
value == true || value == false | ||
end | ||
|
||
# Typecast a value to a true or false | ||
# | ||
# @param [Integer, #to_str] value | ||
# value to typecast | ||
# | ||
# @return [Boolean] | ||
# true or false constructed from value | ||
# | ||
# @api private | ||
def typecast_to_primitive(value) | ||
BOOLEAN_MAP.fetch(value, value) | ||
end | ||
end # class Boolean | ||
end # class Property | ||
end # module DataMapper |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,24 +1,17 @@ | ||
module DataMapper | ||
class Property | ||
class Class < Object | ||
include PassThroughLoadDump | ||
load_as ::Class | ||
dump_as ::Class | ||
coercion_method :to_constant | ||
|
||
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) | ||
DataMapper::Ext::Module.find_const(model, value.to_s) | ||
# @api semipublic | ||
def typecast(value) | ||
DataMapper::Ext::Module.find_const(model, value.to_s) unless value.nil? | ||
rescue NameError | ||
value | ||
end | ||
|
||
end # class Class | ||
end # class Property | ||
end # module DataMapper |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,45 +1,10 @@ | ||
module DataMapper | ||
class Property | ||
class Date < Object | ||
include PassThroughLoadDump | ||
include Typecast::Time | ||
load_as ::Date | ||
dump_as ::Date | ||
coercion_method :to_date | ||
|
||
primitive ::Date | ||
|
||
# Typecasts an arbitrary value to a Date | ||
# Handles both Hashes and Date instances. | ||
# | ||
# @param [Hash, #to_mash, #to_s] value | ||
# value to be typecast | ||
# | ||
# @return [Date] | ||
# Date constructed from value | ||
# | ||
# @api private | ||
def typecast_to_primitive(value) | ||
if value.respond_to?(:to_date) | ||
value.to_date | ||
elsif value.is_a?(::Hash) || value.respond_to?(:to_mash) | ||
typecast_hash_to_date(value) | ||
else | ||
::Date.parse(value.to_s) | ||
end | ||
rescue ArgumentError | ||
value | ||
end | ||
|
||
# Creates a Date instance from a Hash with keys :year, :month, :day | ||
# | ||
# @param [Hash, #to_mash] value | ||
# value to be typecast | ||
# | ||
# @return [Date] | ||
# Date constructed from hash | ||
# | ||
# @api private | ||
def typecast_hash_to_date(value) | ||
::Date.new(*extract_time(value)[0, 3]) | ||
end | ||
end # class Date | ||
end # class Property | ||
end # module DataMapper |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,44 +1,10 @@ | ||
module DataMapper | ||
class Property | ||
class DateTime < Object | ||
include PassThroughLoadDump | ||
include Typecast::Time | ||
load_as ::DateTime | ||
dump_as ::DateTime | ||
coercion_method :to_datetime | ||
|
||
primitive ::DateTime | ||
|
||
# Typecasts an arbitrary value to a DateTime. | ||
# Handles both Hashes and DateTime instances. | ||
# | ||
# @param [Hash, #to_mash, #to_s] value | ||
# value to be typecast | ||
# | ||
# @return [DateTime] | ||
# DateTime constructed from value | ||
# | ||
# @api private | ||
def typecast_to_primitive(value) | ||
if value.is_a?(::Hash) || value.respond_to?(:to_mash) | ||
typecast_hash_to_datetime(value) | ||
else | ||
::DateTime.parse(value.to_s) | ||
end | ||
rescue ArgumentError | ||
value | ||
end | ||
|
||
# Creates a DateTime instance from a Hash with keys :year, :month, :day, | ||
# :hour, :min, :sec | ||
# | ||
# @param [Hash, #to_mash] value | ||
# value to be typecast | ||
# | ||
# @return [DateTime] | ||
# DateTime constructed from hash | ||
# | ||
# @api private | ||
def typecast_hash_to_datetime(value) | ||
::DateTime.new(*extract_time(value)) | ||
end | ||
end # class DateTime | ||
end # class Property | ||
end # module DataMapper |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.