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.

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

Conversation

ThatcherC
Copy link

Adds timezones for DateTime parsing! This is accomplished through the use of the TimeZones.jl package. The tests in test/spec-02-22.expected had to be updated to output ZonedDateTime objects instead of DateTime objects, but I think this change better reflects the strictly time-zoned nature of the input strings (like 2001-12-15T02:59:43.1Z and 2001-12-14t21:59:43.10-05:00).

One question I have for the maintainers - should IANA timezone names like America/New_York be implemented? I don't see mention of them in the YAML spec, but TimeZones.jl supports them and it might be a nice addition.

@kescobo
Copy link
Collaborator

kescobo commented Apr 17, 2023

Sorry for the very long delayed response on this. I think this should be included, even with the additional dependency. Seems like I'm the only maintainer of this package even though I never use it 🤦. If you can rebase onto master so that tests will run, I'm happy to merge this.

@ThatcherC
Copy link
Author

Awesome, no worries! Just rebased. The test run well locally, though it looks like they aren't running here - let me know if there's anything I need to do to get that running. Thanks for checking this out!

@codecov
Copy link

codecov bot commented Apr 18, 2023

Codecov Report

Patch coverage: 88.88% and no project coverage change.

Comparison is base (6d1b676) 86.46% compared to head (a03b28f) 86.46%.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #123   +/-   ##
=======================================
  Coverage   86.46%   86.46%           
=======================================
  Files          12       12           
  Lines        1625     1626    +1     
=======================================
+ Hits         1405     1406    +1     
  Misses        220      220           
Impacted Files Coverage Δ
src/YAML.jl 94.59% <ø> (+0.15%) ⬆️
src/constructor.jl 86.01% <88.88%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@kescobo
Copy link
Collaborator

kescobo commented Apr 18, 2023

Looks great! Going to merge. I think I'll make a minor version release with #130 , and then a major release with this since it's a new feature / adds a dependency.

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

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants