Skip to content

fix(duration): parse decimal commas in duration strings - #3215

Open
Hugohong258 wants to merge 1 commit into
iamkun:devfrom
Hugohong258:work/round-five
Open

Hugohong258 wants to merge 1 commit into
iamkun:devfrom
Hugohong258:work/round-five

Conversation

@Hugohong258

Copy link
Copy Markdown

The duration parser's regular expression already accepts commas in numeric components, but converting those captures directly with Number() produces NaN. For example, dayjs.duration('PT1,5S').asMilliseconds() returns 0 instead of 1500.

Normalize decimal commas to points before converting captured strings to numbers. Add tests for comma/point equivalence across duration units, negative seconds and the normalized ISO output.

Validation: the two new tests fail with the original implementation. After the fix, all 32 duration tests pass, ESLint passes for the changed files, and the full Jest run with coverage passes all 796 tests across 93 suites. The separate multi-time-zone npm script matrix was not run.

@lbesecker195 lbesecker195 left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

DURATION_REGEX_PARSE on dev already accepts [0-9,.] in each component, but Number('1,5') is NaN, so PT1,5S, P1,5Y and PT-1,5S parse to NaN components today. With the replace they become 1.5 / 1.5 / -1.5, matching the point forms. ISO 8601 allows a comma as the decimal sign, and a malformed PT1,5,5S is still NaN either way since only one comma per component is normalized. LGTM.

This branch has not been deployed

No deployments
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.

2 participants