-
Notifications
You must be signed in to change notification settings - Fork 1.1k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: remove hours limit for parseDuration
#7064
base: main
Are you sure you want to change the base?
Conversation
parseDuration
parseDuration
parseDuration
d4f886c
to
f2c9d8a
Compare
- Removes 23-hour limit on duration parsing - Allows parsing of durations like "PT36H" to work correctly - Aligns behavior with Temporal.Duration spec which has no upper limit on hours
f2c9d8a
to
971799e
Compare
a79adcf
to
3013156
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems fine on the surface, will double check with others if there was a reason we did this internally
message from original implementer
Seems like there was a discrepancy in the specs, but I think we're ok to go ahead with this, though we should really do it for all the fields asap, otherwise we're going to have a weird state in the API during the transition |
Looks like we should remove the limits on all of the segments, not just hours. This should also parse:
|
I agree! Would it make sense to set the max for |
Closes #5416
As mentioned in the issue, I reviewed the
Temporal.Duration
spec to align the behavior. It seems that the hours value can go up to 130 and beyond, with no explicit maximum limit. Therefore, I’ve adjusted the maximum value to beInfinity
.The previously invalid case
P18Y7MT30H15S
is valid inTemporal
spec, as it returns:As a result, I’ve updated the test case accordingly.
Additional Notes: Currently, only the
hours
property has had its maximum value limit removed, but properties likeyears
,months
,minutes
, andseconds
should also not have maximum limits. To fully align with theTemporal.Duration
specification, I think it’s necessary to remove the maximum value restrictions for all these properties.✅ Pull Request Checklist:
📝 Test Instructions:
🧢 Your Project: