-
Notifications
You must be signed in to change notification settings - Fork 184
Overhaul duration calculation functions #294
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
Conversation
e4ec4d3
to
08941f8
Compare
For "x days ago", a new standalone function is made that takes the original dates into account. For durations, the rounding to nearest unit function now works in a more consistent way.
08941f8
to
dc727cd
Compare
Thanks for working on this! This PR seems too large to reasonably review and looking through some of the changes seem to not really change logic but just refactor for code style which makes it harder to review. Is it possible to break this PR down into smaller pieces and optimise for a clean diff?
Part of the reason for this was to separate out the logic so that it falls inline with Temporal, so that we may one day be rid of this code and instead use the built-in |
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.
I've just left a couple of comments as jumping off points.
Overall I'm excited by this change but I am also a little concerned about how much is changing, especially with the tests. I'm in no way suggesting the current tests are especially correct but I also think we need to assess each one to see if the new value makes more sense, especially which when it comes to calendars (vs time) can be more of a colloquial preference than a hard science. For example we have found users tend to prefer the calendar year being used for such long durations, meaning Jan 2024-Sep 2022 is often rounded up to the much larger "2 years ago" - if we mark it as "1 year ago" we tend to get complaints 😕.
r.setHours(r.getHours() + duration.hours) | ||
r.setMinutes(r.getMinutes() + duration.minutes) | ||
r.setSeconds(r.getSeconds() + duration.seconds) | ||
const actions: Array<() => void> = [ |
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.
I can't see why we need an array here. Because of the if
? Having an array of functions and reversing them seems costly compared to duplicating the small number of lines twice in an if/else
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.
If the duration to be applied is negative, the order of the operations will be reversed. This is so that adding a duration then subtracting the same duration always gives the original date, which was not the case with the old implementation.
opts?: Partial<RoundingOpts>, | ||
): [number, Intl.RelativeTimeFormatUnit] { | ||
const rounded = roundToSingleUnit(duration, opts) | ||
export function getRoundedRelativeTimeUnit(rounded: Duration): [number, Intl.RelativeTimeFormatUnit] { |
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.
If this is removing an unused variable that might be a great candidate for a smaller PR which can be approved and merged in isolation.
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.
As I have explained initially this is because the new implementation does not rely on a reference date to be passed anymore, but the original implementation did, and so this change can only come along with the new implementation of the function.
@@ -1,6 +1,7 @@ | |||
import DurationFormat from './duration-format-ponyfill.js' | |||
import type {DurationFormatOptions} from './duration-format-ponyfill.js' | |||
const durationRe = /^[-+]?P(?:(\d+)Y)?(?:(\d+)M)?(?:(\d+)W)?(?:(\d+)D)?(?:T(?:(\d+)H)?(?:(\d+)M)?(?:(\d+)S)?)?$/ | |||
const durationRoundingThresholds = [Infinity, 11, 28, 21, 55, 55, 900] |
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.
I'm a little concerned that this array is a level of indirection which makes it hard to figure out the rounding rules. At minimum it might be worth adding comments to each of these explaining that they're months, days, hours and so-on. (Though I'd prefer to keep them localised to their call-sites for better readability).
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.
I just attempted to place the constants at the same place that others appear to be placed, which is to the top of the source file. If putting them next to the function that uses them is better then I will happily do that, in fact I also do prefer it that way. I will be sure to add comments.
Also in case you did not notice, these numbers are in an array because the new rounding routine is generalized, the duration values are put in an array and changed with a loop.
This is a complete rework of the logic, you can see the functions for elapsed time and rounding totally reimplemented with little resemblance to the original code, or you can try comparing outputs from the new implementation against the current one. The vast majority of changes are in the test suites, which I have also rebuilt to more comprehensively test the new behavior.
I could split this into multiple parts if that is what you desire, namely the new elapsed time calculation, the new rounding function, the relative time implementation, and the improved duration application function. However adopting them one by one may lead to inconsistent behavior, unless the splitting is just for the purpose of more convenient code review. |
This is exactly the case. I am happy for us to release as one but it would be nice to get more detail about the changes in a few separate PRs especially when it comes to the tests. |
To specifically address this, my new implementation does output this duration as "in 2 years"/"2 years ago", the code only looks at the year difference in this case.
I completely understand this and so to make it clear I am not in any rush to get this new behavior merged right away, it will of course take some time to discuss all the little details. |
Thank you for the confirmation. This will be changed to just a draft then and as I make the new set of PRs with more detailed descriptions it will be closed in favor of them. |
Closing in favor of split pull requests. |
The current implementation of the relative date display appears to be brittle and actually being broken, for instance with this PR's date being 2 Dec 2024, the date 28 Jan 2024 is being very wrongly displayed as "9 days ago".
This pull request is an attempt to rebuild the computations from the ground up so as to be more consistent and robust:
applyDuration
andelapsedTime
now give consistent results.Some semantics had to be changed and so I expect questions and probably some alterations to the details.