Create ISO 8601 Date/Time Values w/o Milliseconds (re-implemented, ready for review) #1670
Conversation
dobon
changed the title
diesieben07
left a comment
diesieben07
left a comment
There was a problem hiding this comment.
Should we also support this for calendar units?
2024-05 is a valid ISO date, referring to the month of may 2025.
src/datetime.js
Outdated
| * @param {boolean} [opts.includeOffset=true] - include the offset, such as 'Z' or '-04:00' | ||
| * @param {boolean} [opts.extendedZone=false] - add the time zone format extension | ||
| * @param {string} [opts.format='extended'] - choose between the basic and extended format | ||
| * @param {string} [opts.precision='milliseconds'] - specify desired time precision: 'hours', 'minutes', 'seconds' or 'milliseconds' |
There was a problem hiding this comment.
This should mention
- that it will truncate the output
- How it interacts with
suppressSecondsandsuppressMilliseconds
There was discussion about this in the prior PR, but no conclusion. I've added another commit to enable precision from Year to Millisecond, and it seems good to me. Do the test cases show the function behaving the way you had in mind @diesieben07 ? I've implemented the changes you've requested and this PR is ready for review. |
fix bugs from tests appease style rules fix test extend precision + suppress tests replace nested ifs with a fall-through switch case appease javascript linter fix logic to pass tests simplify logic to remove switch case. enable precision for iso dates too refactor to minimize code changes simplify and fix comment typo update comments per code review.
dobon
force-pushed
the
to-iso-truncate-time
branch
from
fc7db09 to
4fc8391
Compare
src/datetime.js
Outdated
| } | ||
| let showSeconds, | ||
| c = ""; | ||
| switch (precisionUnitIndex) { |
There was a problem hiding this comment.
I'm not a fan of the code duplication as well as the magic index numbers here.
There was a problem hiding this comment.
Thanks for the feedback.
Both the loop unraveling(code duplication) and magic index numbers were implemented like that for performance (ie: to minimize comparisons) because toISO can be a hot path.
The loop unravelling was to avoid unnecessary compares on smaller units (eg: if precision is 'days', then don't bother checking if precision is greater than years and months), and the magic index numbers were to facilitate numeric compares instead of string.
That being said, if performance isn't paramount then I agree that the repetitive code reads not-great and I'm happy to re-implement any way you please. As such, I've pushed a commit that passes precision as a string parameter (eliminating the magic numbers), and eliminates the loop unraveling in exchange for a couple more compares. Let me know what you think.
There was a problem hiding this comment.
I agree that toISO should be fast. But we shouldn't make claims about performance without a benchmark :D
|
Thank you! |
3 participants
This PR duplicates and updates PR: #748 and issue: #651
I've incorporated the feedback from that review and PR, minus renaming "supress[milli]second" which should be in a different PR if the change is made.