DateTime comparisons do not work

[TylerPachal]

Here is an example of where DateTime values are not being considered equal, I suppose because under-the-hood a regular == comparison is being done:

# DateTime with no milliseconds
iex(1)> dt_1 = ~U[2020-01-01T00:00:00Z]    
~U[2020-01-01 00:00:00Z]

# DateTime with milliseconds
iex(2)> dt_2 = ~U[2020-01-01T00:00:00.000Z]
~U[2020-01-01 00:00:00.000Z]

# Putting them in maps, they are not equal
iex(3)> MapDiff.diff(%{dt: dt_1}, %{dt: dt_2})
%{
  added: %{dt: ~U[2020-01-01 00:00:00.000Z]},
  changed: :map_change,
  removed: %{dt: ~U[2020-01-01 00:00:00Z]},
  value: %{
    dt: %{
      added: ~U[2020-01-01 00:00:00.000Z],
      changed: :map_change,
      removed: ~U[2020-01-01 00:00:00Z],
      struct_name: DateTime,
      value: %{
        calendar: %{changed: :equal, value: Calendar.ISO},
        day: %{changed: :equal, value: 1},
        hour: %{changed: :equal, value: 0},
        microsecond: %{
          added: {0, 3},
          changed: :primitive_change,
          removed: {0, 0}
        },
        minute: %{changed: :equal, value: 0},
        month: %{changed: :equal, value: 1},
        second: %{changed: :equal, value: 0},
        std_offset: %{changed: :equal, value: 0},
        time_zone: %{changed: :equal, value: "Etc/UTC"},
        utc_offset: %{changed: :equal, value: 0},
        year: %{changed: :equal, value: 2020},
        zone_abbr: %{changed: :equal, value: "UTC"}
      }
    }
  }
}

# Comparing them using == they are not equal
iex(4)> dt_1 == dt_2
false

# Comparing them with DateTime.compare/2 works as expected
iex(5)> DateTime.compare(dt_1, dt_2)
:eq

---

Possible solutions

1. Add an option for common structs like this to be compared using their own compare functions:

MapDiff.diff(map1, map2, compare_carefully: [DateTime, NaiveDateTime])

2. Add a callback to let users do their own comparisons for specific types:

MapDiff.diff(map1, map2, compare: {DateTime, fn a, b -> DateTime.compare(a, b) == :eq end})

If you let me know what your preference is I can prepare a PR.

[Qqwy]

Thank you for sharing this issue! map_diff predates the compare/2-functionality that nowadays exists in Elixir, so indeed there was no thought put into handling this case back then.

I like the freedom of proposed solution (2.), but we will need to support a list of possibilities there as well.
I think the following makes sense:

• MapDiff.diff/3 allows the option compare:, which takes a list.
• Each element of this list is either {StructModuleName, function}, or the shorthand StructModuleName which will work the same as {StructModuleName, &(StructModuleName.compare(&1, &2) == :eq)}.
• Any time we encounter two datatypes which have the same struct type, we check whether the struct's type name is in the compare list.
  • If it is, we use the custom comparison function
  • If not, we fall back to using === (as 1.0 and 1 are considered different).

What do you think?
A PR would be very welcome! 💚

[Qqwy]

If we alter the default from what it is now (using ===) then it would be a breaking change.
I'm not sure that is a good idea.
Another consideration is that we could hard-code the compare for a couple of built-in types, but not for all types out there that do have a compare-function in their module, as it is only a convention and there might be structs out there that have a function called compare that should not be used for Enum.sort nor MapDiff.diff.

So at least for the time being I'd like to keep the default as-is.

[TylerPachal]

Hmm even the code is simple to understand I think this might be bit more work than I can handle right now.

---

A DateTime value is a struct, I was going to add the following but wasn't sure what you would want for the non-equal case.
Maybe this should actual be in the compare function?

def diff(%DateTime{} = a, %DateTime{} = b) do
    case DateTime.compare(a, b) do
      :eq -> %{changed: :equal, value: a}
      _ -> # TODO: What to put here?  We probably don't want to drill down into the individual fields of the struct
    end
  end

---

The Elixir version of 1.3 is giving me compiler errors when trying to add the optional parameters, upgrading from Elixir 1.3 is probably a good idea:

def diff(map_a, map_b, opts \\ [])

---

The very first clause which checks for equality will need to be removed, because it would evaluate two child DateTimes as not being equal without checking the :compare option, resulting in a potential false negative.

def diff(a, a), do: %{changed: :equal, value: a}

---

[Qqwy]

I was going to add the following but wasn't sure what you would want for the non-equal case.

In that case, we should return %{changed: :primitive_change, removed: a, added: b}.

The Elixir version of 1.3 is giving me compiler errors when trying to add the optional parameters, upgrading from Elixir 1.3 is probably a good idea:

👍 Let's drop support for any version older than 1.7.

The very first clause which checks for equality will need to be removed, because it would evaluate two child DateTimes as not being equal without checking the :compare option, resulting in a potential false negative.

I think that the case where two things which are structurally equal are not considered equal according to a compare-implementation very unlikely, but it probably is better to move this case down to after the point where we've checked whether a and b are structs.

[Qqwy]

Let me know how it goes. If you feel like you've bitten off more than you can chew, I will happily jump in to help with the code, but of course I also don't want to tread on your turf 😊 !