Skip to content
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

addressing issue #92 #93

Merged
merged 4 commits into from
Mar 30, 2017
Merged

Conversation

henryw374
Copy link

addressing issue #92

widdh added 2 commits March 29, 2017 12:08
…nt the same date, but which have different internal times are incorrectly compared
@@ -23,20 +24,13 @@
IEquiv
(-equiv [o other]
(and (instance? goog.date.Date other)
(.equals o other)))
(ct/equal? o other)))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure of the reason for this change. Is it necessary?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

felt like it's better to lean on the cljs-time library to check for equality rather than the underlying goog.date lib ... for example if cljs-time changed to use some other way to check for equality, then this would remain consistent with that. Not religious about it tho...

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I see the point.

But I'd rather not change it for the sake of it. Could we leave this out, and revisit if it becomes an issue?

@@ -11,7 +11,8 @@
(:import
[goog.date Date]
[goog.date DateTime]
[goog.date UtcDateTime]))
[goog.date UtcDateTime])
(:require [cljs-time.core :as ct]))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all (or most) parts of the lib are using the alias time

    [cljs-time.core :as time]

Could you change it for consistency?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sure

@andrewmcveigh andrewmcveigh merged commit 4fb58f5 into andrewmcveigh:master Mar 30, 2017
@andrewmcveigh
Copy link
Owner

Merged, thanks very much for this 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants