Conversation
Create the `time.ts` module as a place to put fime functions. Move two time functions there and create tests for them. The `humanizeUnit` function now uses ECMAscript apis. This ensures that pluralization happens appropriately. Also, fix a small bug in the results view to enure `repository` is correctly pluralized.
angelapwen
left a comment
angelapwen
left a comment
There was a problem hiding this comment.
✅ assuming the line I commented on makes sense 😸
| {humanizeDuration(lastUpdated)} | ||
| {humanizeDuration(lastUpdated && -lastUpdated)} |
There was a problem hiding this comment.
I'm not sure I understand this line of code — could you explain it?
There was a problem hiding this comment.
I was about to ask the same question! What does it mean to && these two numbers? 😅
There was a problem hiding this comment.
lastUpdated may be undefined. If so, do not want to negate it (this would convert it to NaN, which is BAD).
If lastUpdated is 0, then negating is a noop. So we don't need to negate.
Would it be easier to understand if I did this?
lastUpdated === undefined ? undefined : -lastUpdated
There was a problem hiding this comment.
| {humanizeDuration(lastUpdated)} | |
| {humanizeDuration(lastUpdated && -lastUpdated)} | |
| {humanizeDuration(lastUpdated === undefined ? undefined : -lastUpdated)} |
There was a problem hiding this comment.
Hm, yes, I find that easier to understand. What happens if lastUpdated is an integer other than 0 though? Do we expect it to be a positive or negative integer (and why do we need to negate it at all if the absolute value is taken in humanizeDuration()?)
There was a problem hiding this comment.
Good questions. If lastUpdated is something other than 0, then it is negated (meaning, a time in the past).
Absolute value is used to determine which unit to use. A positive integer indicates "XXX units from now". A negative integer indicates "XXX units ago".
Maybe it would be clearer if we ensured that lastUpdated was always negative and I can put a comment there.
There was a problem hiding this comment.
oh yes! I see how it works with the new change now. I think that would make it clearer, yes!
| numeric: 'auto', | ||
| }); | ||
|
|
||
| // All these are approximate, specifically months and years |
There was a problem hiding this comment.
Nit
| // All these are approximate, specifically months and years | |
| // Months and years are approximate |
| return ''; | ||
| } | ||
|
|
||
| if (Math.abs(diffInMs) < HOUR_IN_MILLIS) { |
There was a problem hiding this comment.
Should we be covering seconds as well?
There was a problem hiding this comment.
It didn't seem to make sense for our purposes.
| }); | ||
| } | ||
|
|
||
| export function humanizeUnit(diffInMs?: number): string { |
There was a problem hiding this comment.
I have a very tired brain, but I can't easily understand the difference between humanizeDuration and humanizeUnit - can we think of different names?
There was a problem hiding this comment.
I'll add comments. I can't think of better names.
Rename, add comments, and extract some local variables.
|
All good now? |
charisk
left a comment
charisk
left a comment
There was a problem hiding this comment.
Sorry for the delay, thought this was approved!
4 participants
Create the
time.tsmodule as a place to put fime functions.Move two time functions there and create tests for them.
The
humanizeUnitfunction now uses ECMAscript apis. This ensuresthat pluralization happens appropriately.
Also, fix a small bug in the results view to enure
repositoryis correctly pluralized.
Checklist
ready-for-doc-reviewlabel there.