feat: Using a single short duration display format #68
Conversation
| hour_string = hours > 0 ? "#{hours} #{'hour'.pluralize(hours)}" : "" | ||
|
|
||
| "#{hour_string} #{minute_string}".strip | ||
| "#{hour_string}:#{minute_string}".strip |
There was a problem hiding this comment.
This implementation yield some odd results, based on how the previous one worked. In the previous implementation, I didn't want to have results like "0h 1m" or "1h 0m", so I made it produce "1m" and "1h" instead, respectively. I did this by setting hour_string or minute string to an empty string if the hour or minute was 0, respectively, up on lines 18 and 19.
All that to say, to simplify this a bit, this line should be "#{'%02d' % hours}:#{'%02d' % minutes}" and lines 18 and 19 removed.
There was a problem hiding this comment.
Thanks @J3RN. I've added a new PR which formats things with leading zeros. I was able to get Postgres (and then the app) running and eyeballed this myself. Now, for instance, a 30 minute duration reads as "00:30" using your specified change. I'd be keen to find a more readable way to write this, but if you're happy, I'm happy.
There was a problem hiding this comment.
@yock Thanks for those, this looks great!
I understand your concern about readability, and it's a concern that I share. RuboCop suggests using hours.format('%02d') over '%02d' % hours (likewise for minutes, of course). Does that seem like a reasonable readability improvement?
There was a problem hiding this comment.
I like it. I'll make the change and push it up.
There was a problem hiding this comment.
@J3RN Fixnum doesn't contain the format method, but it is on Kernel. The change I pushed up calls format('%02d', hours) instead.
There was a problem hiding this comment.
Whoops! Good call. These changes look ready-to-merge now!
|
Fixes #67 |
re: #67