A 11393 fix specs and ignore commas #3
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Change examples to use work days Remove weeks from examples as we don't support them anymore Remove empty context Move short minute from m to min
bpaul
commented
Mar 23, 2021
| @@ -47,7 +47,7 @@ def output(seconds, opts = {}) | |||
| month = 22 * day | |||
| year = 260 * day | |||
|
|
|||
| if seconds >= 31557600 && seconds%year < seconds%month | |||
| if seconds >= year && !opts[:limit_to_hours] | |||
There was a problem hiding this comment.
Not sure how this ever worked before.
- The
31557600refers to a full year not a working year. seconds%year < seconds%monthjust doesn't make sense to me
There was a problem hiding this comment.
I have no idea what seconds%year < seconds%month means!
bpaul
commented
Mar 23, 2021
| @@ -151,6 +151,7 @@ def calculate_from_words(string, opts) | |||
|
|
|||
| def cleanup(string) | |||
| res = string.downcase | |||
| res = res.gsub(/[0-9]*\,?[0-9]+/) { |m| m.gsub(',', '') } | |||
There was a problem hiding this comment.
This is my fix for the thousands delimiter ie 1,000 seconds. See test below
| work_day = 8 * hour | ||
| work_month = 22 * work_day | ||
| work_year = 260 * work_day | ||
| work_week = 5 * work_day |
There was a problem hiding this comment.
Update specs to use work days, weeks, months and years
| @@ -82,84 +87,68 @@ | |||
| @exemplars = { | |||
| (60 + 20) => | |||
| { | |||
| :micro => '1m20s', | |||
| :short => '1m 20s', | |||
There was a problem hiding this comment.
We use min not m as our short minute delimiter
bpaul
requested review from
jjbohn,
a team and
philwilt
and removed request for
a team
philwilt
approved these changes
Mar 24, 2021
| @@ -47,7 +47,7 @@ def output(seconds, opts = {}) | |||
| month = 22 * day | |||
| year = 260 * day | |||
|
|
|||
| if seconds >= 31557600 && seconds%year < seconds%month | |||
| if seconds >= year && !opts[:limit_to_hours] | |||
There was a problem hiding this comment.
I have no idea what seconds%year < seconds%month means!
jjbohn
approved these changes
Apr 6, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I updated the gem to ignore the thousands delimiters in input strings.
As part of this I fixed the specs which hadn't been updated since we moved to work-days. While fixing the specs I found a bug in our handling of longer timeframes (ie year plus) and fixed that too