Skip to content
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

[core] Duration toIsoString() not following ISO Standard #9906

Closed
irborrero opened this issue Aug 22, 2020 · 3 comments · Fixed by #9935
Closed

[core] Duration toIsoString() not following ISO Standard #9906

irborrero opened this issue Aug 22, 2020 · 3 comments · Fixed by #9935
Assignees
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.

Comments

@irborrero
Copy link

Reproduction Steps

const duration = Duration.days(14)
const stringDuration = duration.toIsoString() 

What did you expect to happen?

stringDuration is `P14D` 

What actually happened?

 stringDuration is `PT14D` 

Environment

  • CDK CLI Version: 1.32.2
  • Node.js Version: 12.18.2
  • OS: macOS Catalina
  • Language: TypeScript

Other

Link to docu

Explanation of my use case

I am creating a Dashboard for my service using CDK. I would like the dashboard to have a time range like:

start time: ~today - 14 days
end time: ~today

I know I can do this by specifying: start: "-P14D"

However I would like to use Duration instead of a string. Something like:

const duration = Duration.days(14)
....
start: "-" + duration.toIsoString()

This is not working because duration.toIsoString() prints PT14D and not P14D .


This is 🐛 Bug Report

@irborrero irborrero added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Aug 22, 2020
@irborrero irborrero changed the title [module] Duration [ toIsoString() not following ISO Standard] Aug 22, 2020
irborrero added a commit to irborrero/aws-cdk that referenced this issue Aug 22, 2020
@irborrero irborrero changed the title Duration [ toIsoString() not following ISO Standard] [core] Duration toIsoString() not following ISO Standard Aug 22, 2020
@github-actions github-actions bot added the @aws-cdk/core Related to core CDK functionality label Aug 22, 2020
@RomainMuller
Copy link
Contributor

Yeah @irborrero's absolutely right... I think I made a bug when I coded this up.

The T in the ISO8601 periods is a token that helps disambiguate the M marker (Months versus Minutes).
From this perspective, the T must be on the right of Days / left of Hours as it must sit between the day and time parts (and it is not needed if there is no time part, only a day part).

rix0rrr added a commit that referenced this issue Aug 24, 2020
We used to render all periods as `PT...`, but the correct
formatting is `P(days)T(hms)`.

Fixes #9906.
@mergify mergify bot closed this as completed in #9935 Aug 25, 2020
mergify bot pushed a commit that referenced this issue Aug 25, 2020
We used to render all periods as `PT...`, but the correct
formatting is `P(days)T(hms)`.

Fixes #9906.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
@voho
Copy link

voho commented Aug 25, 2020

Thanks a lot for fixing this! I am quite new to CDK dev process, are you going to merge this into the older CDK versions as well?

@skinny85
Copy link
Contributor

Thanks a lot for fixing this! I am quite new to CDK dev process, are you going to merge this into the older CDK versions as well?

No, we don't modify existing released versions. You will have to upgrade your CDK version to one that contains the change (in this case, 1.61.0 or later) to get the fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
@aws-cdk/core Related to core CDK functionality bug This issue is a bug. needs-triage This issue or PR still needs to be triaged.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants