search

andrewmcveigh / cljs-time

A clj-time inspired date library for clojurescript.
342 stars 57 forks source link

Equality between local-date and local-date-time is not symmetric #102

Closed dmarjenburgh closed 7 years ago

dmarjenburgh commented 7 years ago

I encountered this weird behaviour:

(def date1 (local-date 2017 1 1)) ;; Any date will do the same
(def date2 (local-date-time 2017 1 1))

(compare date1 date2) ;=> 0
(= date1 date2) .     ;=> true
(compare date2 date1) ;=> 0
(= date2 date1) .     ;=> false(!)

I don't know if this is an error in goog.date.DateTime or cljs-time.

danielcompton commented 7 years ago

This looks like this might be because = looks for IEquiv, and ClojureScript defines this for js/Date

(extend-type js/Date
  IEquiv
  (-equiv [o other]
    (and (instance? js/Date other)
         (== (.valueOf o) (.valueOf other))))

  IComparable
  (-compare [this other]
    (if (instance? js/Date other)
      (garray/defaultCompare (.valueOf this) (.valueOf other))
      (throw (js/Error. (str "Cannot compare " this " to " other))))))

It looks like goog.date.DateTime#equals and goog.date.Date#equals are not symmetrical and this is the source of the problems. I don't have a completely clear idea of exactly where this is happening, but I think it will be in this vicinity.

If you require extend.cljs then your dates will be compared in a more sensible manner.

andrewmcveigh commented 7 years ago

Thanks for digging into this @danielcompton. IIRC, equality on goog.date objects has never worked in "the clojure way", and its behaviour has changed over the years. It's why extend.cljs[1] and cljs-time.internal.core/= [2] exist.

This can't be fixed in cljs-time, but @danielcompton's suggestion of requiring cljs-time.extend is probably the best thing you can do.

[1] https://github.com/andrewmcveigh/cljs-time/blob/v0.5.1/src/cljs_time/extend.cljs#L23-L59 [2] https://github.com/andrewmcveigh/cljs-time/blob/v0.5.1/src/cljs_time/internal/core.cljs#L18