-
Notifications
You must be signed in to change notification settings - Fork 53
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
Add custom comparison for Montrose::{Schedule/Recurrence} #148
base: main
Are you sure you want to change the base?
Conversation
In our application using Montrose, we have some instances where we want to be able to check to see if a schedule has changed. The specific use case for us is with a Rails model that is serializing a schedule - we want to be able to know if the schedule itself has changed, ie: `event.schedule_changed? #=> false` The same attributes may be assigned for the schedule's configuration - but Rails thinks that the schedule has changed b/c it's comparing the objects themselves instead of the underlying recurrence configurations. This commit overloads `Montrose::Schedule#==` so that schedules can be compared against each other within the context of their configurations.
0c6ed63
to
ffa86a8
Compare
In the same vein as adding `Montrose::Schedule#==` This allows `Montrose::Recurrence` objects to be compared by their underlying hash options.
ffa86a8
to
d1d60bd
Compare
Something to note here: In each case, I'm allowing someone to pass in an Array (for I started writing specific spec cases for this, but I couldn't get minitest to operate how I would have expected it to. schedule = new_schedule.tap do |schedule|
schedule << {every: :month}
schedule << {every: :day}
end
identical_array = [
{every: :month},
{every: :day}
]
schedule == identical_array # => true
assert_equal schedule, identical_array # => true
_(schedule).must_equal identical_array # => 💣 I'd love to add specs here - but I'm not quite sure why |
Thank you for the PR! I like the idea of implementing I don't think we will equate with Array and Hash. Callers can still cast accordingly, e.g., As for the def ==(other)
if other.is_a?(self.class)
all?(&:==)
else
super
end
end |
Just offering some drive-by feedback on this while I'm evaluating using montrose in my project. A coercion method would be useful here to convert the def ==(other)
Recurrence(other).all?(&:==)
end That method might look like: def Recurrence(object)
if object.is_a?(Recurrence)
return object
else
Montrose.recurrence(object)
end
end The same could be done for Schedule. I'm not sure if that implementation is exactly correct for this library but it would function like the EDIT: Here's how I'm patching this with an initializer require "montrose"
module RecurrenceComparision
def ==(other)
Montrose.recurrence(other).to_hash == to_hash
end
end
Montrose::Recurrence.prepend(RecurrenceComparision) |
In our application using Montrose, we have some instances where we want
to be able to check to see if a schedule has changed.
The specific use case for us is with a Rails model that is serializing a
schedule - we want to be able to know if the schedule itself has
changed, ie:
event.schedule_changed? #=> false
The same attributes may be assigned for the schedule's configuration -
but Rails thinks that the schedule has changed b/c it's comparing
the objects themselves instead of the underlying recurrence
configurations.
This commit overloads
Montrose::Schedule#==
so that schedules can becompared against each other within the context of their configurations.
Edit:
In addition, I added the same comparison for
Montrose::Recurrence