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

Comparison of LocalDateTime instances in different timezones (and maybe different calendars too) #912

Closed
justingrant opened this issue Sep 16, 2020 · 5 comments
Labels
meeting-agenda zoned-type Part of the effort for a type with timestamp+timezone

Comments

@justingrant
Copy link
Collaborator

In #625 we recently decided to handle the "compare dates in different calendars" problem by creating two methods" date.prototype.equals for strict comparison and date.prototype.equalsISO for comparing just the ISO fields, ignoring calendars.

In last week's meeting when we made this decision, I didn't think about how this would affect LocalDateTime where there's not only a calendar but also a time zone to compare. I apologize for cracking this topic open again.

I guess we could offer 4 equality methods on LocalDateTime:

  • equals - all fields must match
  • equalsISO - ignore calendar
  • equalsUTC? equalsAbsolute? - ignore calendar and timezone
  • ??? - ignore timezone but not calendar. This one seems like a much less common use case, relative to the other three.

Ugh. My first impression is that I'm not excited about 4 different comparison methods, even if we could figure out intuitive names for the last two.

Because one of LocalDateTime's design goals is to be a superset of the DateTime API, if we stick with the decision from #625 then we're committed to at least two of these: equals and equalsISO.

What's unclear is whether we should ignore the time zone in these two methods, add more methods, add options, etc.

FWIW, the current design of LocalDateTime.compare ignores timezone (because it uses Absolute.compare under the covers) and calendar. This is different from how DateTime.compare works which is to return false if the calendars are different.

My initial (not strongly held) opinion would be to do this:

  • equals - all fields must match (to match DateTime.compare behavior)
  • equalsISO - ignore calendar, but timezone must match
  • Don't offer any equalsXxx methods that ignore time zone. Users can always use Temporal.Absolute.compare(one.toAbsolute(), two.toAbsolute()).
  • compare would do "object equality" as defined in Comparison and equality of dates with different calendars #625, where a zero is returned only if all fields match. It'd sort first by toAbsolute().getEpochNanoseconds(), next by calendar.id (to match DateTime.compare behavior), and finally by TimeZone.id.

Feedback appreciated.

@justingrant justingrant added meeting-agenda zoned-type Part of the effort for a type with timestamp+timezone labels Sep 16, 2020
@ptomato
Copy link
Collaborator

ptomato commented Sep 17, 2020

I'm assuming that the two common use cases are going to be: "are these objects identical" (equals), and "are these the same date/time/instant" (ignore calendar but not time zone).

If that assumption holds, then my preference is to keep equals() as is, and add equalsISO() to LocalDateTime and the other types, but under another name like sameInstant(). I would also be OK with the other idea mentioned in today's meeting, to add an options bag to equals(), though it's not my preference. I'm strongly opposed to pushing this outside of Temporal, since I think that both of these use cases will be quite common.

I agree with the proposed behaviour for LocalDateTime.compare.

@sffc
Copy link
Collaborator

sffc commented Sep 17, 2020

I'm okay with the idea of sameInstant(), but can we come up with a name that doesn't have "instant" in it given the decision in #602? There is no concept of "instants" in DateTime and derived types.

@ptomato
Copy link
Collaborator

ptomato commented Sep 17, 2020

Yep, I'd actually prefer something like sameTime() though I thought that would be incongruous for Temporal.Date.prototype.sameTime().

@ptomato ptomato added this to the Complete design decisions milestone Sep 17, 2020
@sffc
Copy link
Collaborator

sffc commented Sep 17, 2020

Some other bikeshed ideas:

  • equalsFuzzy
  • equalsMoment
  • sameMoment
  • isEquivalentTo
  • partialEquals

Another observation: another pattern for doing this comparison would be

date1.difference(date2).equals(/* empty duration */)

Is that too verbose? It might be fine. We could consider adding Temporal.Duration.prototype.isEmpty(), such that you can do

date1.difference(date2).isEmpty()

@ptomato
Copy link
Collaborator

ptomato commented Sep 18, 2020

Meeting, Sept. 18: Due to lack of support, we will drop this. There is disagreement about whether equalsISO() and the other methods besides equals() present enough of a use case to justify their inclusion. equalsISO(), whether under that name or another name, can be considered for inclusion in a future proposal to extend Temporal.

Aside from that, it seems like a good idea to add Duration.prototype.isZero (not "empty"). (→ #924)

@ptomato ptomato closed this as completed Sep 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
meeting-agenda zoned-type Part of the effort for a type with timestamp+timezone
Projects
None yet
Development

No branches or pull requests

3 participants