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

Merge from and fromString methods #252

Draft
wants to merge 3 commits into
base: main
from

Conversation

@ryzokuken
Copy link
Member

ryzokuken commented Nov 1, 2019

Merge the from and fromString methods on the various builtins.

  • Absolute
  • DateTime
  • Date
  • YearMonth
  • MonthDay
  • Time
ryzokuken added 2 commits Nov 1, 2019
Merge `Absolute.from` and `Absolute.fromString` into a single
`Absolute.from` method that has a typecheck underneath.
Merge `DateTime.from` and `DateTime.fromString` into a single
`DateTime.from` method that has a typecheck underneath.
@ryzokuken ryzokuken added this to the November Meeting milestone Nov 1, 2019
@ryzokuken ryzokuken requested review from Ms2ger, littledan and pipobscure Nov 1, 2019
@ryzokuken ryzokuken self-assigned this Nov 1, 2019
@ryzokuken ryzokuken added this to In progress in Finalize spec text via automation Nov 1, 2019
@ryzokuken ryzokuken added this to In progress in Finalize polyfill via automation Nov 1, 2019
Finalize spec text automation moved this from In progress to Reviewer approved Nov 2, 2019
Finalize polyfill automation moved this from In progress to Reviewer approved Nov 2, 2019
const result = ES.CastAbsolute(...args);
static from(arg) {
let result;
if (typeof arg === 'object') {

This comment has been minimized.

Copy link
@ljharb

ljharb Nov 2, 2019

Member

we discussed this in IRC; but presumably you don't want Object('foo') to fall into this path?

This comment has been minimized.

Copy link
@ryzokuken

ryzokuken Nov 2, 2019

Author Member

@ljharb sorry, I didn't apply those changes yet (hence the draft status of the PR), but I'll go about it as discussed.

This comment has been minimized.

Copy link
@littledan

littledan Nov 4, 2019

Member

My (weakly held) intuition is that we shouldn't special-case string wrappers here. If we're doing function overloading distinguishing different kinds of objects, then I'm a bit skeptical of grouping it all into a single from method. Overall, I'd suggest just checking of the typeof is "string", if so do the string handling, otherwise Object() it and get the properties.

This comment has been minimized.

Copy link
@ljharb

ljharb Nov 5, 2019

Member

The only precedent I'm aware of for "takes two types of things" involves doing a brand/protocol check (like IsRegExp, or checking if it's an iterator), and then falling back to a naive coercion. In this case, the only object that's branded here is a string primitive/boxed String, so I leaned towards checking for that.

If that seems weird, then having two methods might be better.

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Nov 6, 2019

Collaborator

If we're not throwing for anything with Type() Object/String, I'd be inclined to check Type() == Object and ToString otherwise.

This comment has been minimized.

Copy link
@littledan

littledan Nov 6, 2019

Member

I don't think this is analogous to things that take iterators or RegExp-likes as arguments; we're not using an object-oriented protocol. The overloading here is more like some other cases, such as the TypedArray constructors, which does what @Ms2ger suggested as a check.

This comment has been minimized.

Copy link
@ljharb

ljharb Nov 6, 2019

Member

Sure, but that only takes a string; it doesn’t also take a bag of properties like this does.

const nanosecond = ES.ToInteger(match[9]);
// ???: why does this exist?
const DateTime = ES.GetIntrinsic('%Temporal.DateTime%');
const Construct = this;

This comment has been minimized.

Copy link
@ljharb

ljharb Nov 2, 2019

Member
Suggested change
const Construct = this;
const Construct = this;
if (!ES.IsConstructor(Construct)) {
throw new TypeError('receiver must be a constructor');
}

This comment has been minimized.

Copy link
@ryzokuken

ryzokuken Nov 2, 2019

Author Member

Addressing #232 should fix this.

microsecond,
nanosecond
});
result = datetime.inZone(zone || 'UTC', match[11] ? match[10] : 'earlier');

This comment has been minimized.

Copy link
@ljharb

ljharb Nov 2, 2019

Member
Suggested change
result = datetime.inZone(zone || 'UTC', match[11] ? match[10] : 'earlier');
result = datetime.inTimeZone(zone || 'UTC', match[11] ? match[10] : 'earlier');

This comment has been minimized.

Copy link
@ryzokuken

ryzokuken Nov 2, 2019

Author Member

How did this even creep in? Thanks.

static from(arg) {
let result;
if (typeof arg === 'object') {
result = ES.CastAbsolute(arg);

This comment has been minimized.

Copy link
@ljharb

ljharb Nov 2, 2019

Member

this should cast to this, not to Absolute

This comment has been minimized.

Copy link
@ryzokuken

ryzokuken Nov 2, 2019

Author Member

Addressing #232 should fix this.

return this === DateTime ? result : new this(result.year, result.month, result.day);
static from(arg) {
if (typeof arg === 'object') {
const result = ES.CastDateTime(arg);

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Nov 4, 2019

Collaborator

I'm not a fan of having CastDateTime and DateTime.from be mutually recursive. Can we move all this into CastDateTime?

Merge `YearMonth.from` and `YearMonth.fromString` into a single
`YearMonth.from` method that has a typecheck underneath.
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.