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

Add spec text #11

Merged
merged 13 commits into from
Jun 2, 2020
Merged

Add spec text #11

merged 13 commits into from
Jun 2, 2020

Conversation

ryzokuken
Copy link
Member

@ryzokuken ryzokuken added this to the Stage 2 milestone May 15, 2020
@ryzokuken ryzokuken self-assigned this May 15, 2020
Copy link
Member

@littledan littledan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a good basis, but some changes are needed, mostly described through inline comments.

In general, don't check in the spec text; that's difficult to maintain. Instead, use gh-actions or another CI system to automatically build it, as other repos do. (However, this is some work and could be done in follow-on patch.)

This initial spec text touches on two of the big design questions that we've been discussing:

  • Do we balance in Intl.DurationFormat, or leave that logic entirely to Temporal? (This text says, leave to Temporal, as I thought was our initial answer)
  • Do we ask constructors to list fields explicitly, or have higher-level upper/lower logic? (This text says, explicitly, but I thought our plan was to start with smallestField/largestField)

Probably the spec text here should be based on smallestField/largestField, which would mean adding some more logic for that. The logic here might be a little subtle, which is why I like having spec text written out, as a design tool for talking through these cases.

spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Show resolved Hide resolved
@ryzokuken ryzokuken marked this pull request as ready for review May 26, 2020 21:52
Copy link
Member

@littledan littledan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't done a fully detailed review, but here are some initial things I noticed.

spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Show resolved Hide resolved
spec.emu Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
Copy link
Member

@littledan littledan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The main thing is improving the logic around dotted. We have to explain how to resolve from the fields to the dotted pattern, including handling cases where there's no dotted pattern for the field set provided.

Overall, I think we have things concrete enough for Stage 2, and this patch can land as is, with future fixes in follow-ons. Getting a concrete spec has helped us get a first draft semantics and enumerate the issues to solve, as it should at this stage.

spec.emu Show resolved Hide resolved
spec.emu Outdated Show resolved Hide resolved
1. Append _patternPart_ to the end of _result_.
1. Else if _duration_[[<_p_>]] is not *undefined* and _p_ exists in _durationFormat_.[[Fields]], then
1. Let _num_ be FormatNumeric(_durationFormat_.[[NumberFormat]], _duration_[[<_p_>]]).
1. Append the new Record { [[Type]]: _p_, [[Value]]: _num_ } to the end of _result_.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should include the individual numeric parts here, not just lump it all together. You can do this by calling PartitionNumberPattern rather than FormatNumeric, as RelativeTimeFormat does.

@ryzokuken ryzokuken merged commit fc8ff13 into tc39:master Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants