Feature/v3.3.2#50
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 selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughThis PR adds custom token evaluation to Tempo formatting, updates compound date tokens to support ChangesTempo tokens and release updates
Sequence Diagram(s)sequenceDiagram
participant format as format(obj?, fmt?, options?)
participant registry as config.registry.tokens[token]
participant tokenfn as custom token function
participant fallback as Tempo-term lookup fallback
format->>registry: look up token
alt config.registry.tokens[token] is a function
registry->>tokenfn: call(zdt, { modifiers, config })
tokenfn-->>format: string | number | bigint
else no custom token function
format->>fallback: resolve #... token name
fallback-->>format: token text
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ 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: 3
🧹 Nitpick comments (2)
packages/tempo/doc/releases/v3.x.md (1)
10-12: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix inconsistent indentation.
Line 12 uses spaces while surrounding lines use tabs. Maintain consistent tab indentation.
🤖 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/releases/v3.x.md` around lines 10 - 12, The Tempo.init configuration snippet has inconsistent indentation, with one line using spaces while the surrounding lines use tabs. Update the indentation in the Tempo.init block to use consistent tab-based alignment throughout, keeping the locale and registry entries formatted uniformly.packages/tempo/src/tempo.type.ts (1)
247-247: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract and strongly type the token registry callback.
The same public callback signature is repeated three times, and
zdt: anyloses the type-safety this API can provide. A shared alias also keeps Options/Config/Discovery from drifting.♻️ Proposed refactor
export namespace Internal { export type Registry = Map<symbol, RegExp> + export type TokenFormatter = ( + zdt: Temporal.ZonedDateTime, + context: { modifiers: string[], config: Config } + ) => string | number | bigint + export type TokenRegistry = Record<string, TokenFormatter>- tokens?: Record<string, (zdt: any, context: { modifiers: string[], config: Internal.Config }) => string | number | bigint>; + tokens?: TokenRegistry;- registry: { formats: FormatRegistry, locales: Record<string, Record<string, string | Function>>, modifiers?: Record<string, string | string[]>, tokens?: Record<string, (zdt: any, context: { modifiers: string[], config: Internal.Config }) => string | number | bigint> }; + registry: { formats: FormatRegistry, locales: Record<string, Record<string, string | Function>>, modifiers?: Record<string, string | string[]>, tokens?: TokenRegistry };- registry?: { formats?: Property<any>, locales?: Record<string, Record<string, string | Function>>, modifiers?: Record<string, string | string[]>, tokens?: Record<string, (zdt: any, context: { modifiers: string[], config: Internal.Config }) => string | number | bigint> }; + registry?: { formats?: Property<any>, locales?: Record<string, Record<string, string | Function>>, modifiers?: Record<string, string | string[]>, tokens?: TokenRegistry };Also applies to: 333-333, 348-348
🤖 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/src/tempo.type.ts` at line 247, The token registry callback signature is duplicated across the public types and currently uses zdt: any, which weakens type safety; extract that callback into a shared alias and reuse it in the tokens definitions for Options/Config/Discovery so they stay aligned. Update the type in tempo.type.ts where tokens is declared to reference the new callback type, and strongly type zdt instead of any using the existing Tempo/date-time type used by this API.
🤖 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/src/module/module.format.ts`:
- Around line 217-219: The per-call token registry handling in module.format.ts
is replacing the base config tokens instead of merging them, so update the
options registry merge logic to deep-merge registry.tokens alongside formats and
locales. In the merge block where the config is built, preserve existing base
tokens from the merged config and combine them with options.registry.tokens so
customTokenFn lookup still works for both defaults and per-call additions. Use
the existing registry/config merge code near the config assembly and the token
resolution in the formatting path as the reference points.
- Around line 217-220: The custom token resolution in module.format.ts should
not invoke inherited prototype members from the registry. Update the lookup
around customTokenFn in the formatting logic to first verify the token exists as
an own property on config?.registry.tokens before treating it as a callback, and
keep the existing isFunction guard before calling it. Use the token handling
branch in the formatter where customTokenFn is read, so plain-object registries
cannot fall through to prototype functions like __defineGetter__.
In `@packages/tempo/test/instance/instance.format.test.ts`:
- Around line 113-124: The compound token formatting test in Tempo does not
actually verify `:year` as a distinct modifier because `{dmy:year}` currently
mirrors `{dmy:yy}` and the `mdy`/`ymd` cases omit `:year` entirely. Update the
`instance.format.test.ts` checks around `Tempo.format` so `:year` has its own
expected output and add matching assertions for `mdy:year` and `ymd:year`, using
the existing `dmy`, `mdy`, and `ymd` format branches to locate the test block.
---
Nitpick comments:
In `@packages/tempo/doc/releases/v3.x.md`:
- Around line 10-12: The Tempo.init configuration snippet has inconsistent
indentation, with one line using spaces while the surrounding lines use tabs.
Update the indentation in the Tempo.init block to use consistent tab-based
alignment throughout, keeping the locale and registry entries formatted
uniformly.
In `@packages/tempo/src/tempo.type.ts`:
- Line 247: The token registry callback signature is duplicated across the
public types and currently uses zdt: any, which weakens type safety; extract
that callback into a shared alias and reuse it in the tokens definitions for
Options/Config/Discovery so they stay aligned. Update the type in tempo.type.ts
where tokens is declared to reference the new callback type, and strongly type
zdt instead of any using the existing Tempo/date-time type used by this API.
🪄 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: 1d468fc0-ff53-4a48-85b6-ebb00e193681
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
package.jsonpackages/library/package.jsonpackages/tempo/CHANGELOG.mdpackages/tempo/doc/releases/v3.x.mdpackages/tempo/doc/tempo.config.mdpackages/tempo/doc/tempo.cookbook.mdpackages/tempo/doc/tempo.format.mdpackages/tempo/package.jsonpackages/tempo/src/module/module.format.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tempo.version.tspackages/tempo/test/instance/instance.format.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
:yycompound date modifier for 2-digit years (e.g.,{dmy:yy}), superseding legacy*6tokens.dmy/mdy/ymdwith:yy/:yearmodifiers.