Skip to content
This repository was archived by the owner on Dec 10, 2021. It is now read-only.

feat: add duration formatter#209

Merged
kristw merged 2 commits intoapache-superset:masterfrom
villebro:duration
Aug 26, 2019
Merged

feat: add duration formatter#209
kristw merged 2 commits intoapache-superset:masterfrom
villebro:duration

Conversation

@villebro
Copy link
Copy Markdown
Contributor

@villebro villebro commented Aug 24, 2019

🏆 Enhancements

  • Add number formatter for time durations based on pretty-ms (MIT license).
  • Add unit tests.

@villebro villebro requested a review from a team as a code owner August 24, 2019 12:31
@villebro
Copy link
Copy Markdown
Contributor Author

villebro commented Aug 24, 2019

For context: We needed a queue time chart on a dashboard, and all existing number formatting options were sub-optimal. For example, 1 min 30 seconds would display as either "90" (seconds) or "1.5" (minutes). This uses pretty-ms to format either milliseconds or seconds into more human readable format:

  • 500 milliseconds = "500ms"
  • 90 seconds = "1m 30s"

Not sure if community PRs are welcomed yet, but thought this might be useful to others, too, so decided to open up a PR.

@netlify
Copy link
Copy Markdown

netlify bot commented Aug 24, 2019

Deploy preview for superset-ui ready!

Built with commit e718a0b

https://deploy-preview-209--superset-ui.netlify.com

@codecov
Copy link
Copy Markdown

codecov bot commented Aug 24, 2019

Codecov Report

Merging #209 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #209      +/-   ##
==========================================
+ Coverage   99.91%   99.91%   +<.01%     
==========================================
  Files         111      112       +1     
  Lines        1239     1245       +6     
  Branches      303      307       +4     
==========================================
+ Hits         1238     1244       +6     
  Partials        1        1
Impacted Files Coverage Δ
...er-format/src/factories/createDurationFormatter.ts 100% <100%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26b690b...e718a0b. Read the comment docs.

Copy link
Copy Markdown
Contributor

@kristw kristw left a comment

Choose a reason for hiding this comment

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

Thank you for submitting. I think this is a nice addition to the library.
I have a few comments below.

Note: After this one eventually merges, it will be part of @superset-ui/number-format@0.12.x you may have to wait for updates to incubator-superset to upgrade from 0.11 to 0.12 which include a few breaking changes that need to be addressed.

const DOLLAR_ROUND = '$,d';
const DOLLAR_ROUND_SIGNED = '+$,d';

const DURATION = 'DURATION';
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please revert the changes to this file. The duration formatter should be optional and not registered by default. There are other apps using this package and some do not need duration formatter. You can register these formatters in incubator-superset's setupFormatter.js. That way other apps can take advantage of tree-shaking and won't have to bundle pretty-ms.

overwritePolicy: OverwritePolicy.WARN,
});

this.registerValue(NumberFormats.DURATION, createDurationFormatter());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Please revert the changes to this file. The duration formatter should be optional and not registered by default. There are other apps using this package and some do not need duration formatter. You can register these formatters in incubator-superset's setupFormatter.js. That way other apps can take advantage of tree-shaking and won't have to bundle pretty-ms when they are not using.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You can add a section in README to instruct how to make duration formatter available.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok that makes sense. I wasn't able to find setupFormatter.js or any reference to getNumberFormatterRegistry() in incubator-superset, but I think I have a good idea of how it should be done. So if it's ok I'll open a new PR to add instructions to README.md once 0.12 is released and durationFormatter gets implemented in the backend.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry I made a typo. It is setupFormatters.js. For README I meant packages/superset-ui-number-format/README.md

@villebro
Copy link
Copy Markdown
Contributor Author

Thanks @kristw for the review and helpful comments. Before I go about making the proposed changes, do you think the name is good (durationFormatter), or should it perhaps be more explicitly related to pretty-ms, i.e. be called prettyMsFormatter? Especially after introducing prettyMsOptions this might seem more appropriate.

@kristw
Copy link
Copy Markdown
Contributor

kristw commented Aug 25, 2019

@villebro I think durationFormatter is quite clear.

@kristw kristw merged commit dcf89a2 into apache-superset:master Aug 26, 2019
kristw pushed a commit that referenced this pull request Apr 17, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants