FormatModule / docs rewrite#44
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughTempo 3.1.1 adds an optional ChangesLibrary: Safe-integer bigint coercion
Tempo format() API enhancements and 3.1.1 release
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/tempo/doc/tempo.cookbook.md (1)
403-413:⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
customDatesample output is now inconsistent with the{h12}template.
{h12}:{mi}is 12-hour output (with auto meridiem), but the example still shows14:30(24-hour).✏️ Suggested doc fix
-console.log(t.format('customDate')); // "2026-06-03 14:30" +console.log(t.format('customDate')); // "2026-06-03 02:30pm"(If you want 24-hour output, switch back to
{hh}:{mi}.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/doc/tempo.cookbook.md` around lines 403 - 413, The custom date format template in the Tempo cookbook example uses the h12 token (which produces 12-hour time format with automatic meridiem) but the expected output still shows 14:30, which is 24-hour format. This is inconsistent. Either update the format template from customDate: '{yyyy}-{mm}-{dd} {h12}:{mi}' to use {hh} instead of {h12} to match the 24-hour output shown, or update the expected output to reflect the correct 12-hour formatted result that would include a meridiem indicator (like 2:30 PM).
🧹 Nitpick comments (1)
packages/tempo/test/discrete/test_upper.ts (1)
1-3: ⚡ Quick winDebug script committed under test directory.
This should be converted into a real
describe/itassertion test (or removed) to avoid dead/noisy test assets.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tempo/test/discrete/test_upper.ts` around lines 1 - 3, The file contains a debug script with a console.log statement instead of a proper unit test. Convert this into a real test using describe/it blocks with assertion statements. Replace the console.log statement that calls t.format('{h12:upper}:{mi}') with an expect assertion that verifies the formatted output matches the expected value, ensuring the test actually validates the behavior rather than just printing debug output.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/tempo/CHANGELOG.md`:
- Around line 9-17: The CHANGELOG.md entry for version 3.1.1 is incomplete and
missing documentation for several substantive format() feature additions. Add
entries to the "Added" section documenting the new {h12} token for 12-hour clock
output with automatic meridiem injection, the {cal} token for calendar system
resolution, and the :raw modifier for stripping leading zeros and suppressing
auto-meridiem. Additionally, add an entry to the "Changed" section documenting
the rewritten automatic meridiem injection logic that now derives modifiers from
the matched hour token. Use the suggested format from the review comment as a
reference for the content.
In `@packages/tempo/doc/tempo.format.md`:
- Line 112: The example output for the `{nano}` timestamp format in the
documentation table incorrectly includes a BigInt suffix `n` at the end of the
nanosecond value. Remove the trailing `n` character from the example output to
correctly represent what the runtime actually emits, which is a plain digit
string without the BigInt literal notation.
In `@packages/tempo/doc/tempo.registry.md`:
- Line 15: In the ISO format registry example comment, replace the `{h12}`
(12-hour hour format) with `{hh}` (24-hour hour format) to correctly represent
ISO-like formatting. The example should show a 24-hour time format, not 12-hour,
to maintain ISO semantics accuracy.
In `@packages/tempo/public/bundle.index.html`:
- Around line 240-243: The import map at line 229 specifies
`@magmacomputing/tempo` version 2, which does not support the {dd:raw} token
syntax being used in the console.log and resEl.textContent assignments at lines
240 and 243. Update the import map URL to reference `@magmacomputing/tempo`
version 3.1.1 or later (changing the version number in the CDN URL from `@2` to
`@3.1.1` or a compatible later version) to enable the raw token modifier syntax
required by the code.
In `@packages/tempo/src/module/module.format.ts`:
- Around line 104-118: The meridiem insertion logic only searches for literal
time tokens like {mi}, {ss}, {ms}, {us}, {ns}, {ff} without accounting for
optional modifiers (e.g., {mi:raw}, {ss:upper}). This causes the anchor point
calculation to miss or misplace the insertion point. Update the miIndex,
ssIndex, and subIndex searches to use regex patterns that match these tokens
with optional modifiers (similar to the existing pattern used for hIndex),
replacing lastIndexOf calls with search calls using patterns like /{mi[^}]*}/
for each respective token, ensuring the rightmost occurrence is found correctly.
- Around line 200-203: The 'raw' case in the format function loses precision for
large numeric values because parseInt converts via Number which has a
MAX_SAFE_INTEGER limit. Replace the parseInt conversion with BigInt to preserve
precision for large numbers like nanosecond values. Change the line to use
BigInt(String(res)).toString() instead of parseInt(String(res), 10).toString()
to ensure that large numeric strings are handled without precision loss.
In `@packages/tempo/test/discrete/debug.test.ts`:
- Around line 4-7: Replace the non-assertive debug code in these test files with
proper assertions. In packages/tempo/test/discrete/debug.test.ts (lines 4-7),
replace the console.log statement with an expect() assertion that validates the
format output for h12:upper and mi formatting. In
packages/tempo/test/discrete/raw.test.js (lines 1-3), test the Tempo :raw output
directly using expect() assertions rather than logging the result of parseInt.
In packages/tempo/test/discrete/locale.test.js (lines 2-3), use a fixed datetime
value and assert that the expected localized output matches the actual output
from Tempo. In packages/tempo/test/discrete/test_upper.ts (lines 1-3), either
convert the code into a proper test case using describe/it blocks with
assertions, or remove it entirely if it is scratch code.
---
Outside diff comments:
In `@packages/tempo/doc/tempo.cookbook.md`:
- Around line 403-413: The custom date format template in the Tempo cookbook
example uses the h12 token (which produces 12-hour time format with automatic
meridiem) but the expected output still shows 14:30, which is 24-hour format.
This is inconsistent. Either update the format template from customDate:
'{yyyy}-{mm}-{dd} {h12}:{mi}' to use {hh} instead of {h12} to match the 24-hour
output shown, or update the expected output to reflect the correct 12-hour
formatted result that would include a meridiem indicator (like 2:30 PM).
---
Nitpick comments:
In `@packages/tempo/test/discrete/test_upper.ts`:
- Around line 1-3: The file contains a debug script with a console.log statement
instead of a proper unit test. Convert this into a real test using describe/it
blocks with assertion statements. Replace the console.log statement that calls
t.format('{h12:upper}:{mi}') with an expect assertion that verifies the
formatted output matches the expected value, ensuring the test actually
validates the behavior rather than just printing debug output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 63204d87-1ea9-4377-bb06-e36d44c2c7b0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (20)
package.jsonpackages/library/package.jsonpackages/tempo/CHANGELOG.mdpackages/tempo/README.mdpackages/tempo/doc/installation.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/doc/tempo.format.mdpackages/tempo/doc/tempo.registry.mdpackages/tempo/package.jsonpackages/tempo/public/bundle.index.htmlpackages/tempo/src/module/module.format.tspackages/tempo/src/tempo.class.tspackages/tempo/test/discrete/debug.test.tspackages/tempo/test/discrete/format.test.tspackages/tempo/test/discrete/locale.test.jspackages/tempo/test/discrete/raw.test.jspackages/tempo/test/discrete/test_upper.tspackages/tempo/test/engine/meridiem.test.tspackages/tempo/test/instance/instance.format.test.tspackages/tempo/test/issues/issue-fixes.test.ts
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
Summary by CodeRabbit
Release Notes - v3.1.1
New Features
{h12}(12-hour format) and{cal}(calendar identifier):raw,:ord,:upper,:lower, and:localeTempo.format()now accepts optional configuration parameter for runtime overridesDocumentation