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

Concerns about options parameter in LocalDateTime.prototype.compare #719

Closed
justingrant opened this issue Jul 1, 2020 · 6 comments
Closed
Labels
zoned-type Part of the effort for a type with timestamp+timezone

Comments

@justingrant
Copy link
Collaborator

justingrant commented Jul 1, 2020

Maybe instead of having the calculation option, we could just remove this method and require you to drop into Absolute or DateTime to do the comparison? That seems in line with the strongly typed approach we take elsewhere.

Originally posted by @ptomato in #700

@justingrant
Copy link
Collaborator Author

Good question. A goal of LocalDateTime was to try to apply the right defaults for the most common use cases. Comparison should almost always use Absolute.compare because it's almost always desired to sort events in the same order as they happened in the real world. Also, getting UTC times out of order can typically lead to much worse consequences than getting clock time values out of order. Therefore, DateTime comparisons should be very rare for this type.

So I don't think it'd be a good tradeoff to remove this method, because it wouldn't make the unusual use case any easier but would make the vast-majority use case much harder in every dimension: more typing, more concepts to learn, and more decisions needed.

Instead, if we're concerned about complexity of this option, I'd suggest removing the option and just always use Absolute comparisons. In the unusual case that a developer needs DateTime comparisons, they can use (and we can document):

ldtArray.sort((ldt1, ldt2) => Temporal.DateTime.compare(ldt1.toDateTime(), ldt2.toDateTime());

This ergonomics is IMHO worse than a usually-defaulted option, but it's a rare use case so i don't feel strongly either way, as long as we keep the nice ergonomics of the main use case which is:

ldtArray.sort(Temporal.LocalDateTime.compare);

BTW, the original design for this method was Absolute-only, but I added the option in response to concerns raised by other champions (@pipobscure maybe?) that defaults in general (not just here) should be overridable. Hence the compare option. But it's also easy enough to take out the option. Let me know what you'd recommend.

@justingrant
Copy link
Collaborator Author

@ptomato replied in #700 (comment):

I wouldn't be opposed to always using Absolute comparison in LocalDateTime. My personal opinion is that extra options are more cognitive load than converting to a different type that does something different. I guess there's a related philosophical question here, is LocalDateTime supposed to become an "everything type" where we say "when in doubt, use LocalDateTime?" Or do we recommend using it in the cases where Absolute and DateTime don't quite fit? My preference would be for the latter and I think removing the comparison method would fit with that approach. Keeping the comparison method or even the option argument would fit with the former approach.

@ptomato ptomato added the zoned-type Part of the effort for a type with timestamp+timezone label Jul 1, 2020
@justingrant
Copy link
Collaborator Author

My personal opinion is that extra options are more cognitive load than converting to a different type that does something different.

In my experience this depends on the developer. Novice developers typically use options only if their initial attempt at using defaults doesn't return valid-looking results. Adding options doesn't add much difficulty for them because they usually ignore options whenever possible. For advanced developers, I'm inclined to agree with you-- a new option to learn is probably harder than being referred by docs to an existing known type, especially for obscure cases like DateTime ordering of real-world instants.

The bigger cognitive load I'm worried about here is the decision point. If we don't have LocalDateTime.compare, then every comparison call and every sort requires the developer to decide between Absolute.compare and DateTime.compare, even though 95%+ of the time Absolute.compare is the right choice. (Maybe 99%+). Forcing decisions like this on novices who barely understand time zones let alone DST is the exact reason why I think we need LocalDateTime to reduce the cognitive load for common tasks including comparison.

I guess there's a related philosophical question here, is LocalDateTime supposed to become an "everything type" where we say "when in doubt, use LocalDateTime?" Or do we recommend using it in the cases where Absolute and DateTime don't quite fit? My preference would be for the latter and I think removing the comparison method would fit with that approach. Keeping the comparison method or even the option argument would fit with the former approach.

Yep, this question has been raised by @sffc and @pipobscure too, so it'd be helpful to come up with a consensus answer.

Could we follow up on this topic in #706 and keep this issue focused on compare?

@justingrant
Copy link
Collaborator Author

Another argument for having compare (with or without an options param) is that the main reason for LocalDateTime is to provide DST-safe math operations, and compare is definitely "math". Omitting compare would make math harder, esp. in cases where durations may be negative.

@ptomato
Copy link
Collaborator

ptomato commented Aug 3, 2020

With the decision philosophy of "LocalDateTime is an event that happened, or will happen, in a particular place" - it seems to me that LocalDateTime should have Absolute compare semantics.

@justingrant
Copy link
Collaborator Author

Yep, agreed. I'm going to close this issue as resolved by removing the options param from this method.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
zoned-type Part of the effort for a type with timestamp+timezone
Projects
None yet
Development

No branches or pull requests

2 participants