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.

Sign up for GitHub

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

Jump to bottom

feat(core): duration.toHumanString() #6691

Merged
merged 6 commits into from
Mar 12, 2020
Merged

feat(core): duration.toHumanString() #6691

merged 6 commits into from
Mar 12, 2020

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Mar 12, 2020

Add a function for Duration to render itself to a human readable
string.

This can be used in dashboards or other situations where Durations need
to be represented.

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

@rix0rrr
feat(core): duration.toHumanString()
4a16d4c 
Add a function for `Duration` to render itself to a human readable
string.

This can be used in dashboards or other situations where Durations need
to be represented.
@rix0rrr rix0rrr requested a review from a team March 12, 2020 10:26
@rix0rrr rix0rrr self-assigned this Mar 12, 2020
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Mar 12, 2020
nija-at
nija-at reviewed Mar 12, 2020
View reviewed changes
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

Since you are in there, could we also clean up -

aws-cdk/packages/@aws-cdk/core/package.json

Lines 60 to 70 in bed5357

"docs-public-apis:@aws-cdk/core.Duration.days",
"docs-public-apis:@aws-cdk/core.Duration.hours",
"docs-public-apis:@aws-cdk/core.Duration.millis",
"docs-public-apis:@aws-cdk/core.Duration.minutes",
"docs-public-apis:@aws-cdk/core.Duration.seconds",
"docs-public-apis:@aws-cdk/core.Duration.toDays",
"docs-public-apis:@aws-cdk/core.Duration.toHours",
"docs-public-apis:@aws-cdk/core.Duration.toISOString",
"docs-public-apis:@aws-cdk/core.Duration.toMilliseconds",
"docs-public-apis:@aws-cdk/core.Duration.toMinutes",
"docs-public-apis:@aws-cdk/core.Duration.toSeconds",

packages/@aws-cdk/core/lib/duration.ts
*/
public toHumanString(): string {
if (this.amount === 0) { return fmtUnit(0, this.unit); }
if (Token.isUnresolved(this.amount)) { return `<token> ${this.unit.label}`; }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not do ${this.amount} {this.unit} instead? The token should get resolved, yes?
It might not be the best output and we might end up something like '458321 seconds' but at least it's something.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, nevermind. I realized we can't do string operations on numeric tokens.

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to error instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you think that's reasonable? I feel it's more reasonable to render what we got, which is " minutes", and let the human decide whether that's good enough for them or not.

If not, they can always add the check themselves.

packages/@aws-cdk/core/lib/duration.ts
Comment on lines +164 to +165
// 2 significant parts, that's totally enough for humans
return parts.slice(0, 2).join(' ');
Copy link
Contributor

Choose a reason for hiding this comment

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

Could be a configurable option, but we can totally do that later.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was thinking the same.

packages/@aws-cdk/core/test/test.duration.ts
test.equal(Duration.seconds(3666).toHumanString(), '1 hour 1 minute');

test.equal(Duration.millis(3000).toHumanString(), '3 seconds');
test.equal(Duration.millis(3666).toHumanString(), '3 seconds 666 millis');
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it do the right thing for Duration.millis(3.6).toHumanString()?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Probably not. You crazy person.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 4a16d4c
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 5df4201
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 0353c16
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@nija-at nija-at added the pr/do-not-merge This PR should not be merged at this time. label Mar 12, 2020
nija-at
nija-at approved these changes Mar 12, 2020
View reviewed changes
Copy link
Contributor

@nija-at nija-at left a comment

Choose a reason for hiding this comment

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

LGTM. Added 'do-not-merge' label in case you want to address the linter exclusions I mentioned, while you're in there.

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 8fdc7fb
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: d13ec37
  • Result: FAILED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@iliapolo iliapolo mentioned this pull request Mar 12, 2020
Merged
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 0604838
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@rix0rrr rix0rrr merged commit d833bea into master Mar 12, 2020
@rix0rrr rix0rrr deleted the huijbers/render-duration branch March 12, 2020 13:22
@snyk-bot snyk-bot mentioned this pull request Apr 10, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nija-at nija-at nija-at approved these changes

Assignees

@rix0rrr rix0rrr

Labels
contribution/core This is a PR that came from AWS. pr/do-not-merge This PR should not be merged at this time.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rix0rrr @aws-cdk-automation @nija-at