Editorial: DRY refactor of time formatting #2629
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
justingrant
force-pushed
the
refactor-FormatSecondsStringPart
branch
from
630ff7a
to
281f1bc
Compare
Codecov Report
@@ Coverage Diff @@
## main #2629 +/- ##
==========================================
- Coverage 95.98% 95.96% -0.02%
==========================================
Files 20 20
Lines 11574 11528 -46
Branches 2201 2193 -8
==========================================
- Hits 11109 11063 -46
Misses 401 401
Partials 64 64
|
gibson042
approved these changes
Jul 16, 2023
justingrant
force-pushed
the
refactor-FormatSecondsStringPart
branch
2 times, most recently
from
c409664
to
9f7e46d
Compare
justingrant
changed the title
|
@gibson042 - I expanded the scope of this PR to align with your feedback in #2607 (comment). I added a new Want to re-review this PR? Thanks! |
gibson042
approved these changes
Jul 17, 2023
There was a problem hiding this comment.
Nice! I think I'd also like to see colon-vs.-empty-string separator parameterization (for FormatOffsetTimeZoneIdentifier), but that might be best placed in a followup. 
justingrant
force-pushed
the
refactor-FormatSecondsStringPart
branch
from
9f7e46d
to
045c2c9
Compare
This editorial commit removes redundant spec text dealing with time formatting: * Adds a new AO FormatTimeString, and calls it in TemporalTimeToString, TemporalDateTimeToString, and TimeString (the legacy date AO in 262). * Removes FormatSecondsStringPart and replaces it with a new AO FormatFractionalSeconds, and call it in FormatTimeString and TemporalDurationToString. The text of this new AO is aligned with similar text in GetOffsetStringFor in tc39#2607. * Replaces sub-second formatting text in TemporalDurationToString with a call to FormatFractionalSeconds. * Aligns polyfill code to these spec changes. * Adjusts polyfill code in a few places to better match the spec. Note that this commit doesn't touch spec text for formatting time zone offsets because there are several in-flight PRs dealing with offsets and I wanted to keep this PR merge-conflict-free. But once those PRs land and the dust settles, then I'll send another editorial PR to DRY offset string formatting too, by using FormatTimeString to replace bespoke formatting text for offsets.
justingrant
force-pushed
the
refactor-FormatSecondsStringPart
branch
from
045c2c9
to
5fa747b
Compare
Yep, was gonna save that for the next PR after all the other normative offset-related PRs have landed. I made all the edits you recommended, so gonna merge this now. Thanks for your help! |
justingrant
added a commit
to justingrant/proposal-temporal
that referenced
this pull request
Jul 18, 2023
Simplify offset formatting by using FormatTimeString instead of bespoke formatting logic. This commit completes the time-formatting refactor that tc39#2629 started.
justingrant
added a commit
to justingrant/proposal-temporal
that referenced
this pull request
Jul 18, 2023
Simplify offset formatting by using FormatTimeString instead of bespoke formatting logic. This commit completes the time-formatting refactor that tc39#2629 started.
justingrant
added a commit
to justingrant/proposal-temporal
that referenced
this pull request
Jul 18, 2023
Simplify offset formatting by using FormatTimeString instead of bespoke formatting logic. This commit completes the time-formatting refactor that tc39#2629 started.
justingrant
added a commit
to justingrant/proposal-temporal
that referenced
this pull request
Jul 18, 2023
Simplify offset formatting by using FormatTimeString instead of bespoke formatting logic. This commit completes the time-formatting refactor that tc39#2629 started.
justingrant
added a commit
to justingrant/proposal-temporal
that referenced
this pull request
Jul 19, 2023
Simplify offset formatting by using FormatTimeString instead of bespoke formatting logic. This commit completes the time-formatting refactor that tc39#2629 started.
justingrant
added a commit
to justingrant/proposal-temporal
that referenced
this pull request
Jul 19, 2023
Simplify offset formatting by using FormatTimeString instead of bespoke formatting logic. This commit completes the time-formatting refactor that tc39#2629 started.
justingrant
added a commit
to justingrant/proposal-temporal
that referenced
this pull request
Jul 19, 2023
Simplify offset formatting by using FormatTimeString instead of bespoke formatting logic. This commit completes the time-formatting refactor that tc39#2629 started.
justingrant
added a commit
to justingrant/proposal-temporal
that referenced
this pull request
Jul 19, 2023
Simplify offset formatting by using FormatTimeString instead of bespoke formatting logic. This commit completes the time-formatting refactor that tc39#2629 started.
justingrant
added a commit
to justingrant/proposal-temporal
that referenced
this pull request
Jul 19, 2023
Simplify offset formatting by using FormatTimeString instead of bespoke formatting logic. This commit completes the time-formatting refactor that tc39#2629 started.
justingrant
added a commit
that referenced
this pull request
Jul 19, 2023
Simplify offset formatting by using FormatTimeString instead of bespoke formatting logic. This commit completes the time-formatting refactor that #2629 started.
moz-v2v-gh
pushed a commit
to mozilla/gecko-dev
that referenced
this pull request
Sep 13, 2023
…=allstarschh Implement the changes from <tc39/proposal-temporal#2629>. Differential Revision: https://phabricator.services.mozilla.com/D185410
vinnydiehl
pushed a commit
to vinnydiehl/mozilla-unified
that referenced
this pull request
Sep 14, 2023
…=allstarschh Implement the changes from <tc39/proposal-temporal#2629>. Differential Revision: https://phabricator.services.mozilla.com/D185410
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This editorial PR removes redundant spec text dealing with time formatting:
Note that this PR doesn't touch spec text for formatting time zone offsets because there are several in-flight PRs dealing with offsets and I wanted to keep this PR merge-conflict-free. But once those PRs land and the dust settles, then I'll send another editorial PR to DRY offset string formatting too, by using FormatTimeString to replace bespoke formatting text for offsets.