Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add timezones parsing #123

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Add timezones parsing #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1acf38b
Added timezones to Spec 02-22 tests
ThatcherC Oct 3, 2022
42ee995
Made test case with explicit "Z" suffix expect a zoned date time
ThatcherC Oct 3, 2022
a260264
Implemented ZonedDateTime output!
ThatcherC Oct 3, 2022
0107e85
Removed old code and comments
ThatcherC Oct 3, 2022
a03b28f
Removed the timezone TODO note
ThatcherC Oct 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ Base64 = "2a0f44e3-6c83-55bd-87e4-b1978d98bd5f"
Dates = "ade2ca70-3891-5945-98fb-dc099432e06a"
Printf = "de0858da-6303-5e67-8744-51eddeeeb8d7"
StringEncodings = "69024149-9ee7-55f6-a4c4-859efe599b68"
TimeZones = "f269a46b-ccf7-5d73-abea-4c690281aa53"

[compat]
StringEncodings = "0.3"
Expand Down
1 change: 0 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,5 +142,4 @@ date: 2012-08-06
* When writing YAML files, you cannot use additional constructors like you can when reading.
* Parsing sexigesimal numbers.
* Fractions of seconds in timestamps.
* Specific time-zone offsets in timestamps.
* Application specific tags.
1 change: 1 addition & 0 deletions src/YAML.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import Base: iterate

using Base64: base64decode
using Dates
using TimeZones
using Printf
using StringEncodings

Expand Down
39 changes: 20 additions & 19 deletions src/constructor.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -327,34 +327,35 @@ function construct_yaml_timestamp(constructor::Constructor, node::Node)
m = parse(Int, mat.captures[5])
s = parse(Int, mat.captures[6])

if mat.captures[7] === nothing
return DateTime(yr, mn, dy, h, m, s)
end

ms = 0

if mat.captures[7] !== nothing
ms = mat.captures[7]
if length(ms) > 3
ms = ms[1:3]
end
ms = parse(Int, string(ms, repeat("0", 3 - length(ms))))
end

delta_hr = 0
delta_mn = 0

if mat.captures[9] !== nothing
delta_hr = parse(Int, mat.captures[9])
end

if mat.captures[10] !== nothing
delta_mn = parse(Int, mat.captures[10])

if mat.captures[8] === nothing
return DateTime(yr, mn, dy, h, m, s, ms)
Copy link
Collaborator

@kescobo kescobo Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually - before we merge, I wonder about this - it makes the function not type-stable. Should this also be a ZonedDateTime, but just with UTC?

In any case, we should have a test case that hits this line I think

Copy link
Author

@ThatcherC ThatcherC Apr 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea! Nice that codecov shows it isn't currently tested. Any idea what the test should be? The other tests are taken exactly form the spec (here) so we'd have to make up a line that has time and date but no timezone.

I'm torn on what to do about the DateTime vs ZonedDateTime thing... I think DateTime is used a lot more, and it is useful even without timezones. Dates.now(), for example, shows my local system time. If I wrote a YAML document and specified times with no consideration for timezone, I don't know if I'd want to the read to assume it was all UTC. ZonedDateTimes are definitely a safer choice though. Unfortunately I don't think the spec makes a clear choice so we'll have to pick an implementation ourselves.

else
tzString = mat.captures[8]

if mat.captures[8] != "Z"

# TimeZones.jl doesn't accept a time zone like "-5",
# so we'll insert an extra zero to get something like "-05"
delta_hr = mat.captures[9]
if length(delta_hr) == 1
tzString = tzString[1]*"0"*tzString[2:end]
end

end

tz = TimeZone(tzString)
ZonedDateTime(yr, mn, dy, h, m, s, ms, tz)
end

# TODO: Also, I'm not sure if there is a way to numerically set the timezone
# in Calendar.

return DateTime(yr, mn, dy, h, m, s, ms)
end


Expand Down
7 changes: 4 additions & 3 deletions test/spec-02-22.expected
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
using Dates
using TimeZones

Dict{Any,Any}(
"canonical" => DateTime(2001, 12, 15, 2, 59, 43, 100),
"iso8601" => DateTime(2001, 12, 14, 21, 59, 43, 100),
"spaced" => DateTime(2001, 12, 14, 21, 59, 43, 100),
"canonical" => ZonedDateTime(2001, 12, 15, 2, 59, 43, 100, TimeZone("00:00")),
"iso8601" => ZonedDateTime(2001, 12, 14, 21, 59, 43, 100, TimeZone("-05:00")),
"spaced" => ZonedDateTime(2001, 12, 14, 21, 59, 43, 100, TimeZone("-05:00")),
"date" => Date(2002, 12, 14)
)