-
Notifications
You must be signed in to change notification settings - Fork 158
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
Remove CalendarToString and TimeZoneToString #1303
Conversation
Also remove %Temporal.Calendar.prototype.toString% intrinsic. These are not needed because toString() is a required method of the Calendar protocol, so there should always be a toString() there to call. See: #1294
Also remove %Temporal.TimeZone.prototype.toString% intrinsic. Similarly to the previous commit, these are not needed because toString() is a required method of the TimeZone protocol, so there should always be a toString() there to call. See: #1294
Codecov Report
@@ Coverage Diff @@
## main #1303 +/- ##
=======================================
Coverage 95.77% 95.77%
=======================================
Files 19 19
Lines 9419 9405 -14
Branches 1445 1443 -2
=======================================
- Hits 9021 9008 -13
+ Misses 392 391 -1
Partials 6 6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fair enough.
@@ -879,7 +867,7 @@ <h1>Temporal.Calendar.prototype.toJSON ( )</h1> | |||
</p> | |||
<emu-alg> | |||
1. Let _calendar_ be the *this* value. | |||
1. Return ? CalendarToString(_calendar_). | |||
1. Return ? ToString(_calendar_). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a reasonable alternative would be to Call %Temporal.Calendar.toString%, but this way plays better with subclassing i suppose.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This way does an additional observable Get(calendar, @@toPrimitive) but I think that's OK?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, either a Symbol.toPrimitive or a toString lookup would be required to invoke subclass code.
See: #1294 (but does not close the discussion)