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 functions #35

Closed
domenic opened this issue Jul 10, 2017 · 33 comments
Closed

Comparison functions #35

domenic opened this issue Jul 10, 2017 · 33 comments
Labels
behavior Relating to behavior defined in the proposal

Comments

@domenic
Copy link
Member

domenic commented Jul 10, 2017

Should comparison be part of the proposal? I realize it's more awkward without operator overloading. And for instants it's easy to convert to numbers and compare. But it seems important to be able to compare PlainDate/PlainTime/PlainDateTime. (Is this day equal to X's birthday? Is the time between 9am and 5pm?)

I think it'd be good if comparison between unlike types threw, at least in the beginning, instead of trying to do some clever conversion.

@apaprocki
Copy link
Contributor

I think it'd be good if comparison between unlike types threw, at least in the beginning, instead of trying to do some clever conversion.

+1 if comparisons are supported

@mattjohnsonpint
Copy link
Collaborator

Yes. I think we should include these. :)

@mattjohnsonpint mattjohnsonpint changed the title Comparison functions? TODO: Comparison functions Jul 10, 2017
@gilmoreorless
Copy link
Contributor

In-built comparison functions would also be very handy when sorting a list of PlainDates.

@maggiepint
Copy link
Member

I would tend to agree with Dominic that some basic comparisons on like types make sense for the initial proposal. Let's make cross-type comparisons something for later or never.

@domenic
Copy link
Member Author

domenic commented Jul 11, 2017

/cc @littledan as I believe he had ideas on how to add named comparison functions now which can in the far future maybe become the basis for overloaded operators.

@maggiepint
Copy link
Member

I do want to note that I'm resisting having this proposal ride on a bunch of other proposals. Both BigInt and operator overloading have come up in this context, and this proposal will be hard enough to get through as it is given the size.

@mattjohnsonpint
Copy link
Collaborator

For sorting, I think as long as we define the result of valueOf (#25) that should work ok.

@domenic
Copy link
Member Author

domenic commented Jul 11, 2017

Array.prototype.sort uses toString, not valueOf.

@mattjohnsonpint
Copy link
Collaborator

Oh really! That's good to know. Thanks. Another good point for #25.

@maggiepint
Copy link
Member

maggiepint commented Jul 11, 2017

How the heck does that work?

new Date().toString()
"Tue Jul 11 2017 11:04:10 GMT-0700 (Pacific Daylight Time)"

That... won't work right. Does it specifically use .toISOString() for dates?

@domenic
Copy link
Member Author

domenic commented Jul 11, 2017

No, you just can't use Array.prototype.sort()'s default comparator on dates effectively.

@maggiepint
Copy link
Member

Apparently I've been using moment too long for sorts and I forgot how dates work :-)

@doodadjs
Copy link

Maybe make an exception (and a correction) for that new date system and make .sort() use .valueOf() ?

@maggiepint
Copy link
Member

Well, that spirals into writing special cases into array.prototype.sort. One could, but I don't know how well received it would be.

@doodadjs
Copy link

That could be a "Symbol.sort" function key used when present instead of .toString() ?

@timrwood
Copy link
Contributor

If the output of PlainTime#toString is an ISO8601 string, it will sort as expected.

["01:00:00", "00:00:00", "00:00:01", "00:01:00"].sort()
["00:00:00", "00:00:01", "00:01:00", "01:00:00"]

If the output of PlainDate#toString is an ISO8601 string, it will sort as expected for 4 digit years, though 5+ digit years would not sort correctly unless left zero filled.

@doodadjs
Copy link

doodadjs commented Jul 11, 2017

Would not it be more efficient to sort by a floating point or decimal value instead of converting to strings and then sort by comparing strings ?

@maggiepint
Copy link
Member

At this point, it doesn't really matter what's most efficient. What matters is that Array.prototype.sort calls .toString() on what is passed to it, and changing that is out of scope for this proposal because it would involve changing other parts of the spec than date.

@keithamus
Copy link
Member

keithamus commented Jul 11, 2017

Wait, wait, there's a mistake here.

Array.prototype.sort()'s default implementation should call Symbol.toPrimitive (spec) on any given values that have it:

a = {name: 'a', [Symbol.toPrimitive]:(hint) => console.log('1', hint) || 2}
b = {name: 'b', [Symbol.toPrimitive]:(hint) => console.log('2', hint) || 1}
[b, a].sort().map(o => o.name)
> 1 string
> 2 string
> ["b", "a"]

This is different to calling .toString(). In theory one could implement a [Symbol.toPrimitive] function that just returns timestamps. Quoth the spec (emphasis mine):

If an object is capable of converting to more than one primitive type, it may use the optional hint PreferredType to favour that type. Conversion occurs according to Table 9:

@domenic
Copy link
Member Author

domenic commented Jul 11, 2017

Yes, sorry, I meant to capitalize ToString, not toString.

@keithamus
Copy link
Member

More problematic, as ToString will force any ToPrimitive into a String type. Looks like operator overloading is needed!

@gilmoreorless
Copy link
Contributor

Well, that'll teach me for not reading the spec! I had no idea Array.prototype.sort used toString().

No, you just can't use Array.prototype.sort()'s default comparator on dates effectively.

Wow, yeah, I guess I'd only been sorting explicit ISO string representations previously, because I hadn't run into that behaviour before. But now that I test it out...

[1, 2, 3].map(n => new Date(2017, 6, n)).sort();
/*
[
  Mon Jul 03 2017 00:00:00 GMT+1000 (AEST),
  Sat Jul 01 2017 00:00:00 GMT+1000 (AEST),
  Sun Jul 02 2017 00:00:00 GMT+1000 (AEST)
]
*/

@littledan
Copy link
Member

About operator overloading: I was thinking that maybe we could start with static methods on constructors, and then upgrade to operator overloading. However, in the design of BigInt, that approach was rejected by people I talked with on the committee in favor of just having the operators overloaded.

The default sort comparison is entirely busted. I don't see a point in trying to overload new dates to make it fixed. As long as there's an effective comparison method that can be called directly, or passed in as an argument, it seems good to me.

If we want to make comparison to other types with < throw, how about making the valueOf method exist and throw? We could patch SortCompare, but really, given how entirely broken it would remain, that seems like "putting lipstick on a pig"; we obviously can't make .toString() throw in general.

@ryzokuken
Copy link
Member

I don’t believe this is something that’s being done in the scope of this project, and therefore am closing it. Please feel free to reopen. Adding the deferred label so we can probably come back to this later.

@domenic
Copy link
Member Author

domenic commented Jul 12, 2019

I don't have privileges to reopen, but I think this should be in the scope of the project.

@kaizhu256
Copy link
Contributor

datetime-comparison would be trivial (w/o overloading) if javascript sticks with [utc] ISOStrings for most of its business-logic -- and this proposal instead focused on static-functions that manipulated [utc] ISOStrings instead of classes.

@ryzokuken ryzokuken reopened this Jul 12, 2019
@ryzokuken
Copy link
Member

@mj1856 @pipobscure @sffc what do you all think about this?

@sffc
Copy link
Collaborator

sffc commented Jul 12, 2019

I think this is within the scope of the proposal if someone can champion this bug and figure out the right way to specify the comparison function.

@littledan
Copy link
Member

I believe the current proposal is to add such a comparison function.

@gibson042 gibson042 added behavior Relating to behavior defined in the proposal needs-spec-text and removed deferred labels Sep 11, 2019
@gibson042
Copy link
Collaborator

Correct, and in fact more than one.

@littledan
Copy link
Member

Can we close this issue, then?

@gibson042 gibson042 changed the title TODO: Comparison functions Comparison functions Sep 12, 2019
@gibson042
Copy link
Collaborator

We still need spec text.

@pipobscure
Copy link
Collaborator

Closing, because we have a decision and we're trying to find what needs to be discussed.

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

No branches or pull requests