Add support for ordinal date values in AS::TimeZone.iso8601 #41779
Conversation
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. |
| zone = ActiveSupport::TimeZone["Eastern Time (US & Canada)"] | ||
|
|
||
| exception = assert_raises(ArgumentError) do | ||
| zone.iso8601("12936") |
There was a problem hiding this comment.
Rather than remove this it’d be good to see a test for what now happens when this value is parsed.
There was a problem hiding this comment.
@ghiculescu No problem, 12936 is another invalid value - actually coming from a real-world case that led me to make this PR. I can revert to using this value as the existing test will work, or test both this and 21367 as invalid ordinal values but that is sort of proving the same point twice.
I changed to 21367 as it was closer to the boundary of a valid/invalid ordinal - the 367 ending produces a yday date part of 367 which makes it invalid. Perhaps it would have been better to have left the original invalid value in place?
There was a problem hiding this comment.
Nah that makes sense, thanks for the explanation.
|
I think this needs a changelog entry. |
steventux
force-pushed
the
add-support-for-iso8601-dates
branch
from
5537522
to
6c08376
Compare
activesupport/CHANGELOG.md
Outdated
| A valid ordinal value will be converted to an instance of `TimeWithZone` using the `:year` | ||
| and `:yday` fragments returned from `Date._iso8601`. | ||
|
|
||
| As requested by [Andrew White](https://github.com/rails/rails/pull/41764#issuecomment-808863720) |
There was a problem hiding this comment.
I think you can remove this line. Better to focus on how it impacts Rails users. So maybe:
| As requested by [Andrew White](https://github.com/rails/rails/pull/41764#issuecomment-808863720) | |
| ```ruby | |
| twz = ActiveSupport::TimeZone["Eastern Time (US & Canada)"].iso8601("21087") | |
| twz.to_a[0, 6] == [0, 0, 0, 28, 03, 2021] | |
| ``` |
There was a problem hiding this comment.
@ghiculescu Thank you for the suggestion - amended.
steventux
force-pushed
the
add-support-for-iso8601-dates
branch
from
6c08376
to
1c5a236
Compare
activesupport/test/time_zone_test.rb
Outdated
| zone = ActiveSupport::TimeZone["Eastern Time (US & Canada)"] | ||
|
|
||
| twz = zone.iso8601("21087") | ||
| assert_equal [0, 0, 0, 28, 03, 2021], twz.to_a[0, 6] |
There was a problem hiding this comment.
Can you change this to match the assertion style in other tests, e.g.
twz = zone.iso8601("21087")
assert_equal Time.utc(2021, 3, 28, 0, 0, 0), twz.time
assert_equal zone, twz.zone
That way we ensure nothing funny happens to fractional seconds
There was a problem hiding this comment.
@pixeltrix thanks for the review - amended.
Date._iso8601 will attempt to parse certain args as ordinal dates eg. '21087' => 2021-03-28. Add ActiveSupport::TimeZone.iso8601 support for these ordinal values to be consistent with Date._iso8601 and raise ArgumentError where the day of year is invalid.
steventux
force-pushed
the
add-support-for-iso8601-dates
branch
from
1c5a236
to
21ca4a4
Compare
|
@steventux thanks 👍🏻 |
Summary
Date._iso8601will attempt to parse certain values as ordinal date strings eg.'21087'is parsed as 28th March 2021.ActiveSupport::TimeZone.iso8601should provide similar support for ordinal values as Ruby stdlib inDate._iso8601.Add support for values which parse as year and day of year eg.
{ year: 2021, yday: 87 }and attempt to generate a valid date withDate.ordinal. This will sanitize the:ydaypart of the parsed value for the corresponding year.Other Information
See #41764 (comment) for suggestion which led to this PR.