search

andrewmcveigh / cljs-time

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

after? and before? for goog.date.Date #92

Closed henryw374 closed 7 years ago

henryw374 commented 7 years ago

Similar issue to #70, before? after? and equal? for goog.date.Date are implemented in terms of .getTime.

(ct/before? (ct/today) (ct/today)) => true

So I was going to submit a pull request, with different implementations of those, using the same methods as were used in #70. but then I was thinking if cljs-time.extend wasn't optional then the impl would be a bit simpler. I guess you have good reason for it being optional, but wanted to check first

andrewmcveigh commented 7 years ago

Hey,

The cljs-time.extend needs to be opt-in because it redefines how equality works for (essentially) built-in data types. Users need to be aware of that opt-in, rather than a library just overriding that kind of functionality across their code base.

I'm surprised that...

(ct/before? (ct/today) (ct/today)) => true

... is the case though. That's totally broken.

henryw374 commented 7 years ago

Ok good point.

Shall I do a pull request?

andrewmcveigh commented 7 years ago

:+1: That would be awesome!

henryw374 commented 7 years ago

Cool. I created https://github.com/andrewmcveigh/cljs-time/pull/93 . Let me know what you think.

I was also thinking if today should return a date which has has it's internal time at midnight. That should be irrelevant though really.

andrewmcveigh commented 7 years ago

Hey,

Generally looks good. I added a couple of comments.

I think today might be broken. If it has time fields (does it? I don't think it used to) then they should be set to midnight.

henryw374 commented 7 years ago

Hi,

goog.date.Date just wraps an actual js/Date which is using ms since epoch.

If today uses the goog.date.Date constructor which accepts a js/Date that would seem better. It sets the internal time to midnight in the local tz, `(and still works with time/do-at) :

  (goog.date.Date. (js/Date. (*ms-fn*))))

I notice the doc on today is the same as clj-time, but now I'm thinking that it is a bit misleading for cljs-time to say 'it doesn't deal with timezones', since the date it returns is the date in the local tz, as opposed to today-at which is UTC. Also it has tz methods like

    (.getTimezoneOffset))

Clearly the problem is the underlying impl of goog.date.Date relying on js/Date, unlike JodaTime. I guess I'd class this as one of those things that you shouldn't need to know ideally, but it might leak out... especially if you're converting from Dates to DateTimes and vice versa as I am doing in de/serializing these.

andrewmcveigh commented 7 years ago

Ah, so today is just wrong. It would be better if it was as you suggest.

I always had the goal to remove goog.date from most of cljs-time, but that's unlikely to happen now. The goog.date.Date stuff is not really nice. It's a problem relying on a lib that's so tied to mutable objects, and js/Date.

henryw374 commented 7 years ago

Yeah I see. #94 changes the impl but also changes the docs a bit... see what you think.

andrewmcveigh commented 7 years ago

Yep, looks good. Merged.

Thanks very much!

henryw374 commented 7 years ago

You're welcome!