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

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

Closed
henryw374 opened this Issue Mar 29, 2017 · 10 comments

Comments

Projects
None yet
2 participants
@henryw374
Copy link

henryw374 commented Mar 29, 2017

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

This comment has been minimized.

Copy link
Owner

andrewmcveigh commented Mar 29, 2017

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

This comment has been minimized.

Copy link
Author

henryw374 commented Mar 29, 2017

Ok good point.

Shall I do a pull request?

@andrewmcveigh

This comment has been minimized.

Copy link
Owner

andrewmcveigh commented Mar 29, 2017

👍 That would be awesome!

@henryw374

This comment has been minimized.

Copy link
Author

henryw374 commented Mar 29, 2017

Cool. I created #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

This comment has been minimized.

Copy link
Owner

andrewmcveigh commented Mar 30, 2017

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

This comment has been minimized.

Copy link
Author

henryw374 commented Mar 30, 2017

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 added a commit that referenced this issue Mar 30, 2017

@andrewmcveigh

This comment has been minimized.

Copy link
Owner

andrewmcveigh commented Mar 30, 2017

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

This comment has been minimized.

Copy link
Author

henryw374 commented Mar 30, 2017

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

@andrewmcveigh

This comment has been minimized.

Copy link
Owner

andrewmcveigh commented Mar 30, 2017

Yep, looks good. Merged.

Thanks very much!

@henryw374

This comment has been minimized.

Copy link
Author

henryw374 commented Mar 30, 2017

You're welcome!

@henryw374 henryw374 closed this Mar 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.