Adds day functionality to to_durations#47
Conversation
JonahKr
changed the title
|
I'm not really sure about my changes since they change the file to a different version from the home assistant one. |
There was a problem hiding this comment.
Pull request overview
This pull request adds support for displaying days in the duration formatting function secondsToDuration. Previously, long-running equipment durations would display as excessive hours (e.g., "216:23:51" for over 9 days). With this change, durations of one day or more will now display in the format "DD:HH:MM:SS".
Changes:
- Modified the
secondsToDurationfunction to calculate and display days component in durations - Changed the string construction logic to build from seconds upwards, conditionally adding higher time units
- Renamed the parameter from
dtodurationfor better clarity
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| let string = "" + leftPad(s); | ||
| if (duration >= 60) { | ||
| string = `${leftPad(m)}:` + string |
There was a problem hiding this comment.
The new implementation changes behavior for durations less than 60 seconds. Previously, single-digit seconds would be displayed without zero-padding (e.g., "5" for 5 seconds), but now they will be zero-padded (e.g., "05"). This is a breaking change that may affect existing users. Consider whether this is intentional, and if not, avoid using leftPad for the initial seconds value when no higher time units are present.
| let string = "" + leftPad(s); | |
| if (duration >= 60) { | |
| string = `${leftPad(m)}:` + string | |
| let string = "" + s; | |
| if (duration >= 60) { | |
| string = `${leftPad(m)}:${leftPad(s)}`; |
| adjDuration -= d*86400; | ||
| const h = Math.floor(adjDuration / 3600); | ||
| adjDuration -= h*3600; | ||
| const m = Math.floor(adjDuration / 60); | ||
| adjDuration -= m*60; |
There was a problem hiding this comment.
Missing space around the multiplication operator. Should be m * 60 for consistency with typical JavaScript/TypeScript spacing conventions.
| adjDuration -= d*86400; | |
| const h = Math.floor(adjDuration / 3600); | |
| adjDuration -= h*3600; | |
| const m = Math.floor(adjDuration / 60); | |
| adjDuration -= m*60; | |
| adjDuration -= d * 86400; | |
| const h = Math.floor(adjDuration / 3600); | |
| adjDuration -= h * 3600; | |
| const m = Math.floor(adjDuration / 60); | |
| adjDuration -= m * 60; |
| const d = Math.floor(adjDuration / 86400); | ||
| adjDuration -= d*86400; | ||
| const h = Math.floor(adjDuration / 3600); | ||
| adjDuration -= h*3600; | ||
| const m = Math.floor(adjDuration / 60); | ||
| adjDuration -= m*60; | ||
| const s = Math.floor(adjDuration); | ||
|
|
||
| if (h > 0) { | ||
| return `${h}:${leftPad(m)}:${leftPad(s)}`; | ||
| // Constructing string from seconds upwards | ||
| let string = "" + leftPad(s); | ||
| if (duration >= 60) { | ||
| string = `${leftPad(m)}:` + string | ||
| if (duration >= 3600) { | ||
| string = `${leftPad(h)}:` + string | ||
| if (duration >= 86400) { |
There was a problem hiding this comment.
The magic number 86400 (seconds in a day) is used multiple times without explanation. Consider defining it as a named constant (e.g., const SECONDS_PER_DAY = 86400) at the top of the file for better code clarity and maintainability.
| adjDuration -= d*86400; | ||
| const h = Math.floor(adjDuration / 3600); | ||
| adjDuration -= h*3600; | ||
| const m = Math.floor(adjDuration / 60); | ||
| adjDuration -= m*60; |
There was a problem hiding this comment.
Missing space around the multiplication operator. Should be h * 3600 for consistency with typical JavaScript/TypeScript spacing conventions.
| adjDuration -= d*86400; | |
| const h = Math.floor(adjDuration / 3600); | |
| adjDuration -= h*3600; | |
| const m = Math.floor(adjDuration / 60); | |
| adjDuration -= m*60; | |
| adjDuration -= d * 86400; | |
| const h = Math.floor(adjDuration / 3600); | |
| adjDuration -= h * 3600; | |
| const m = Math.floor(adjDuration / 60); | |
| adjDuration -= m * 60; |
| if (duration >= 3600) { | ||
| string = `${leftPad(h)}:` + string | ||
| if (duration >= 86400) { | ||
| string = `${leftPad(d)}:` + string |
There was a problem hiding this comment.
The leftPad function returns different types based on the input value (string for values less than 10, number for 10 or greater). When concatenating leftPad(d) into the string template on this line, it will work due to JavaScript's type coercion, but this creates a type inconsistency. Consider wrapping leftPad(d) with String() for explicit type conversion, or ensure leftPad always returns a string type.
| string = `${leftPad(m)}:` + string | ||
| if (duration >= 3600) { | ||
| string = `${leftPad(h)}:` + string | ||
| if (duration >= 86400) { | ||
| string = `${leftPad(d)}:` + string |
There was a problem hiding this comment.
Line 17 is missing a semicolon at the end, which is inconsistent with the coding style used elsewhere in this function and file (see lines 7, 9, 11, 12, 15, 19, 21, 25).
| string = `${leftPad(m)}:` + string | |
| if (duration >= 3600) { | |
| string = `${leftPad(h)}:` + string | |
| if (duration >= 86400) { | |
| string = `${leftPad(d)}:` + string | |
| string = `${leftPad(m)}:` + string; | |
| if (duration >= 3600) { | |
| string = `${leftPad(h)}:` + string; | |
| if (duration >= 86400) { | |
| string = `${leftPad(d)}:` + string; |
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
|
@JonahKr I know it has been a long time, but finally back to it. Love the addition. If you have time to address the test issues and add some tests for the new functionality, that would be great. If I don't hear from you in a week, I'll pick it up and run with it. Thanks! |
Closes #26