rossta / montrose

Recurring events library for Ruby. Enumerable recurrence objects and convenient chainable interface.
https://rossta.net/montrose/
MIT License
821 stars 53 forks source link

Add custom comparison for Montrose::{Schedule/Recurrence} #148

Open thewatts opened 2 years ago

thewatts commented 2 years ago

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.

Edit:

In addition, I added the same comparison for Montrose::Recurrence

thewatts commented 2 years ago

Something to note here:

In each case, I'm allowing someone to pass in an Array (for Schedule) or Hash (for Recurrence).

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 .must_equal is having this effect, as I've (perhaps wrongly) assumed that assert_equal and must_equal are identical:

https://github.com/seattlerb/minitest/blob/3c6576a51f4e266996e3459d7a0dd054eb4c87f7/lib/minitest/expectations.rb#L38

rossta commented 2 years ago

Thank you for the PR! I like the idea of implementing #== for Schedule and Recurrence.

I don't think we will equate with Array and Hash. Callers can still cast accordingly, e.g., schedule == Schedule.new(array) and recurrence == Recurrence.new(hash).

As for the Schedule implementation, how about we delegate to Recurrence#==, something like this:

def ==(other)
  if other.is_a?(self.class)
    all?(&:==)
  else
    super
  end
end
saturnflyer commented 5 months ago

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 other object so that you can depend on the methods defined there.

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 Array() method.

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)