-
Notifications
You must be signed in to change notification settings - Fork 180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Refactor Relation
and conditions to fix mutating scopes issues
#268
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
class ActiveHash::Relation::Condition | ||
attr_reader :constraints, :inverted | ||
|
||
def initialize(constraints) | ||
@constraints = constraints | ||
@inverted = false | ||
end | ||
|
||
def invert! | ||
@inverted = !inverted | ||
|
||
self | ||
end | ||
|
||
def matches?(record) | ||
match = begin | ||
return true unless constraints | ||
|
||
expectation_method = inverted ? :any? : :all? | ||
|
||
constraints.send(expectation_method) do |attribute, expected| | ||
value = record.public_send(attribute) | ||
|
||
matches_value?(value, expected) | ||
end | ||
end | ||
|
||
inverted ? !match : match | ||
end | ||
|
||
private | ||
|
||
def matches_value?(value, comparison) | ||
return comparison.any? { |v| matches_value?(value, v) } if comparison.is_a?(Array) | ||
return comparison.cover?(value) if comparison.is_a?(Range) | ||
return comparison.match?(value) if comparison.is_a?(Regexp) | ||
|
||
normalize(value) == normalize(comparison) | ||
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. Could you talk more on this? The first thing I tested was: (1.0).to_s == 1.to_s But cases like this work great (which are probably more relevant since this will fix the very common "4".to_s == "4".to_s 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. Yes; this is to make ActiveHash behave similar to how ActiveRecord behaves for querying. ActiveRecord (or rather the DB) does a similar typecasting when querying: # AR
User.find("2").id # => 2
User.find_by(id: "2").id # => 2
User.find_by(username: :john).username # => "john" The # AH
City.find("2").id # => 2
City.find_by(id: "2").id # => 2
City.find_by(country_code: :gb).country_code # => "gb" .. without this normalization, no records would be returned in the examples above for AH, while it would for AR. |
||
end | ||
|
||
def normalize(value) | ||
value.respond_to?(:to_s) ? value.to_s : value | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
class ActiveHash::Relation::Conditions | ||
attr_reader :conditions | ||
|
||
delegate :<<, :map, to: :conditions | ||
|
||
def initialize(conditions = []) | ||
@conditions = conditions | ||
end | ||
|
||
def matches?(record) | ||
conditions.all? do |condition| | ||
condition.matches?(record) | ||
end | ||
end | ||
|
||
def self.wrap(conditions) | ||
return conditions if conditions.is_a?(self) | ||
|
||
new(conditions) | ||
end | ||
end |
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.
is there a reason to change this block?
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 suppose the
Relation
could be initialized with the conditions directly.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.
The initializer has:
I wondered if there was a reason to not use that.
The new code passes a nil (so it gets and empty array)
and the
where!
then adds to that conditions.Is this to ensure we don't modify the
options[:conditions]
?I kinda like the original way better but if this has a reason then 👍