(434) strptime_with_mock_date :: Fix year boundary #437
Conversation
|
@jchapa thank you, this is a great contribution! I am going to make a few suggestions through the GH review feature. |
| year = d[:year] || d[:cwyear] || now.year | ||
|
|
||
| # If "current" time falls near a year boundary, with a year-ambiguous str, we need to explicitly handle it | ||
| cwday = d[:cwday] && (now.to_date + (d[:cwday] - now.to_date.wday)) |
There was a problem hiding this comment.
the to_date calls on now are not necessary. it will be easier to read without those.
|
|
||
| # Test for cweek | ||
| def test_date_strptime_year_boundary_with_cweek | ||
| # TODO: year-ambiguous cweek is not handled correctly during year boundaries: |
There was a problem hiding this comment.
i do not understand this TODO.
There was a problem hiding this comment.
I think you referenced this in your other comment, but I think in general we're just not handling cweek well in this library. I'm not sure if that's something we want to tackle in this PR.
Some examples (from master branch):
irb(main):003> Date.strptime('1992-52', '%G-%V')
=> #<Date: 1992-12-28 ((2448985j,0s,0n),+0s,2299161j)>
irb(main):004> Date.strptime_without_mock_date('1992-52', '%G-%V')
=> #<Date: 1992-12-21 ((2448978j,0s,0n),+0s,2299161j)>
irb(main):001> Date.strptime('52', '%V')
=> #<Date: 2025-12-29 ((2461039j,0s,0n),+0s,2299161j)>
irb(main):002> Date.strptime_without_mock_date('52', '%V')
=> #<Date: 2025-12-22 ((2461032j,0s,0n),+0s,2299161j)>
Going to poke at this a little more and hopefully find a cleaner solve
|
|
||
| # Test for wnum0 | ||
| def test_date_strptime_year_boundary_with_wnum0 | ||
| assert_equal Date.strptime('52', '%U'), Date.new(1992, 12, 27) |
There was a problem hiding this comment.
Date.new(1992,12,27) is sort of a magic number, I had to do research to understand why it was the right date. What do you think of using Date.strptime('1992-52', '%Y-%U') instead of newing up that date? This comment is general to all the magic dates in here.
There was a problem hiding this comment.
BTW if you implement this idea then for the cweek tests you have to use %G not %Y like Date.strptime('1992-52', '%G-%V'). I spent a bit of time scratching my head at this
3.2.2 :016 > Date.strptime('1992-53', '%Y-%V')
=> #<Date: 1992-01-01 ((2448623j,0s,0n),+0s,2299161j)>
before i realized that the iso8601 commercial week has its own year concept too.
There was a problem hiding this comment.
Date.new(1992,12,27) is sort of a magic number, I had to do research to understand why it was the right date. What do you think of using Date.strptime('1992-52', '%Y-%U') instead of newing up that date? This comment is general to all the magic dates in here.
I like this idea. 2 caveats:
- We would lose some coverage if we compared the output of this function to another output of this function. e.g. what if a future change breaks how the year is computed more broadly in strptime which impacts both sides of the assertion? In that scenario, we'd be blind to other types of changes. we could cover those with addl tests, but I think there's some advantage to using explicit constants in these tests.
- We use magic numbers in our other strptime tests. We could update these though, but would also introduce the same risk.
For these specific tests, if I don't change the other strptime tests, then I think we could safely improve readability making the change you suggested.
Fix for #434. For dates within a week of a year boundary, calling
strptime_with_mock_datewith only a cwday or a wday was causing the wrong year to be used.