weekOfYear#47
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughVersion 3.2.2 of the Tempo library renames the ISO week-of-year accessor from ChangesTempo v3.2.2: ISO week rename and format token cleanup
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)
✏️ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/tempo/doc/tempo.format.md (1)
101-137:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winToken table is still missing part of the new public surface.
Around Line 101, docs add
{h24}, but the table should also prioritize{wy}(rename target) and include{dmy6},{mdy6},{ymd6}so docs match the formatter behavior shipped in this release.💡 Suggested doc patch
-| `{ww}` | Zero-padded ISO Week of Year | `43` | +| `{wy}` | Zero-padded ISO Week of Year | `43` | @@ | `{dmy}` | Compact Date (ddmmyyyy) | `24102026` | | `{mdy}` | Compact Date (mmddyyyy) | `10242026` | | `{ymd}` | Compact Date (yyyymmdd) | `20261024` | +| `{dmy6}` | Compact Date (ddmmyy) | `241026` | +| `{mdy6}` | Compact Date (mmddyy) | `102426` | +| `{ymd6}` | Compact Date (yymmdd) | `261024` |🤖 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.format.md` around lines 101 - 137, The token documentation table starting around line 101 is incomplete and does not include all tokens that are part of the public API. Add the missing `{wy}` token (which should be prioritized in the table) and the three compact date tokens `{dmy6}`, `{mdy6}`, and `{ymd6}` to the token table. Each token should have a descriptive name, explanation of what it does, and a concrete example value consistent with the existing token entries in the table so the documentation accurately reflects the formatter behavior shipped in this release.packages/tempo/src/module/module.format.ts (1)
169-180:⚠️ Potential issue | 🟠 MajorImport and use
getISOWeekOfYearhelper for week tokens to prevent undefined output.Week tokens (
yw,yyww,yywy,ww,wy) directly accesszdt.yearOfWeekandzdt.weekOfYearon lines 169, 170, and 179. This bypasses thegetISOWeekOfYearhelper (defined ininternational.library.ts) that provides a fallback ISO week calculation for environments where Temporal returns undefined week fields.Fix: Use the ISO week helper
-import { formatDayPeriod, getDTF, getPR } from '`#library/international.library.js`'; +import { formatDayPeriod, getDTF, getPR, getISOWeekOfYear } from '`#library/international.library.js`'; const result = template.replace(new RegExp(Match.formatBraces, 'g'), (_match: string, fullToken: string) => { const [token, ...modifiers] = fullToken.split(':'); + const isoWeek = getISOWeekOfYear(zdt); let res: any; switch (token) { - case 'yw': res = pad(zdt.yearOfWeek, 4); break; - case 'yyww': case 'yywy': res = pad(zdt.yearOfWeek, 4) + pad(zdt.weekOfYear); break; + case 'yw': res = pad(isoWeek.yearOfWeek, 4); break; + case 'yyww': case 'yywy': res = pad(isoWeek.yearOfWeek, 4) + pad(isoWeek.weekOfYear); break; - case 'ww': case 'wy': res = pad(zdt.weekOfYear); break; + case 'ww': case 'wy': res = pad(isoWeek.weekOfYear); break;🤖 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/module/module.format.ts` around lines 169 - 180, The week token cases ('yw', 'yyww', 'yywy', 'ww', 'wy') are directly accessing zdt.yearOfWeek and zdt.weekOfYear properties, which may return undefined in certain environments. Replace these direct property accesses with calls to the getISOWeekOfYear helper function from international.library.ts, which provides proper fallback ISO week calculation. Ensure the helper is imported at the top of the file and use it to safely retrieve both year of week and week of year values for all affected token cases.
🤖 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/library/src/common/international.library.ts`:
- Around line 155-166: The fallback ISO week computation uses calendar-dependent
properties (year, dayOfYear, dayOfWeek) on the PlainDate object, which will
produce incorrect results when the input has a non-ISO calendar. After creating
the pd constant from zdt, normalize it to the ISO 8601 calendar using the
withCalendar method before performing the shift and nearestThursday
calculations. This ensures that the year, dayOfYear, and dayOfWeek properties
reflect ISO 8601 values, making the subsequent yearOfWeek and weekOfYear
calculations correct regardless of the input calendar.
In `@packages/tempo/CHANGELOG.md`:
- Around line 9-13: The CHANGELOG entry for version 3.2.2 is incomplete and only
documents the removal of uppercase format tokens. Add documentation for the
additional breaking changes and new features in this release: add an entry
documenting the renaming of the `ww` token to `wy`, document the new short-year
tokens that were added, and include the new date format tokens `{dmy6}`,
`{mdy6}`, and `{ymd6}`. This information should be added as separate subsections
(such as "Changed" for the token rename and "Added" for new tokens) within the
3.2.2 release section to provide complete migration context for users upgrading
to this version.
In `@packages/tempo/src/support/support.enum.ts`:
- Line 87: The ELEMENT enum and elementKeys still reference the old `ww` token
while the yearWeek format definition at line 87 has been updated to use the new
`wy` token. Update all occurrences of `ww` in the ELEMENT enum entries (around
lines 217 and 221) to use `wy` instead to ensure consistency between the format
token surface and the public enum contract.
In `@packages/tempo/src/tempo.class.ts`:
- Around line 1376-1378: The removal of the `ww` getter property in favor of
`wy` is a breaking API change for a patch release. To maintain backward
compatibility, add back the `ww` property as a deprecated getter that returns
the same value as the new `wy` getter (calling getISOWeekOfYear and returning
weekOfYear). Additionally, ensure that the type alias `ww` is also exposed
alongside `wy` at line 1670 where the public API types are defined, so both
aliases remain available for existing consumers.
In `@packages/tempo/src/tempo.type.ts`:
- Line 148: The removal of the exported `ww` type alias on line 148 breaks
source-level compatibility for TypeScript consumers in a patch release. Add back
the `ww` type alias as a deprecated export that maps to the same IntRange type
as the new `wy` alias, maintaining backward compatibility while allowing
consumers to migrate to the new naming over time.
---
Outside diff comments:
In `@packages/tempo/doc/tempo.format.md`:
- Around line 101-137: The token documentation table starting around line 101 is
incomplete and does not include all tokens that are part of the public API. Add
the missing `{wy}` token (which should be prioritized in the table) and the
three compact date tokens `{dmy6}`, `{mdy6}`, and `{ymd6}` to the token table.
Each token should have a descriptive name, explanation of what it does, and a
concrete example value consistent with the existing token entries in the table
so the documentation accurately reflects the formatter behavior shipped in this
release.
In `@packages/tempo/src/module/module.format.ts`:
- Around line 169-180: The week token cases ('yw', 'yyww', 'yywy', 'ww', 'wy')
are directly accessing zdt.yearOfWeek and zdt.weekOfYear properties, which may
return undefined in certain environments. Replace these direct property accesses
with calls to the getISOWeekOfYear helper function from
international.library.ts, which provides proper fallback ISO week calculation.
Ensure the helper is imported at the top of the file and use it to safely
retrieve both year of week and week of year values for all affected token cases.
🪄 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: 1c2bd382-95f8-4aa2-8b24-efaf61507563
📒 Files selected for processing (11)
package.jsonpackages/library/package.jsonpackages/library/src/common/international.library.tspackages/tempo/CHANGELOG.mdpackages/tempo/doc/tempo.format.mdpackages/tempo/package.jsonpackages/tempo/src/module/module.format.tspackages/tempo/src/support/support.enum.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/src/tempo.version.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/library/src/common/object.library.ts`:
- Around line 115-121: The deepMerge function is vulnerable to prototype
pollution because it assigns values to arbitrary keys without validating them.
Before assigning a key-value pair in the forEach loop within deepMerge, add a
check to skip assignment for dangerous keys: __proto__, constructor, and
prototype. Only allow safe keys to be merged into the target object by adding a
guard condition that prevents these special property names from being assigned,
preventing potential prototype mutation attacks when untrusted data is passed to
the function.
🪄 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: 935a14fc-5029-406c-9faf-847caf2b086a
📒 Files selected for processing (15)
packages/library/src/common/international.library.tspackages/library/src/common/object.library.tspackages/tempo/CHANGELOG.mdpackages/tempo/doc/tempo.format.mdpackages/tempo/src/module/module.format.tspackages/tempo/src/support/support.default.tspackages/tempo/src/support/support.enum.tspackages/tempo/src/support/support.init.tspackages/tempo/src/support/support.intl.tspackages/tempo/src/tempo.class.tspackages/tempo/src/tempo.type.tspackages/tempo/test/core/static.test.tspackages/tempo/test/discrete/format.test.tspackages/tempo/test/engine/timezone_offset.test.tspackages/tempo/test/instance/instance.format.test.ts
💤 Files with no reviewable changes (2)
- packages/tempo/src/support/support.intl.ts
- packages/tempo/test/discrete/format.test.ts
✅ Files skipped from review due to trivial changes (3)
- packages/tempo/test/instance/instance.format.test.ts
- packages/tempo/test/engine/timezone_offset.test.ts
- packages/tempo/CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/tempo/src/tempo.class.ts
- packages/tempo/src/module/module.format.ts
- packages/library/src/common/international.library.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
New Features
wydmy6,mdy6,ymd6(2-digit years)Changes
ww→wy;wwremains deprecatedh24token and refinedh12/meridiem and ISO week token formattingintlsettings are mergedBug Fixes
Documentation
{h12}vs{h24}guidance