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

Create timezone-aware datetimes when parsed as such #163

Merged
merged 8 commits into from Dec 15, 2019
Merged

Create timezone-aware datetimes when parsed as such #163

merged 8 commits into from Dec 15, 2019

Conversation

@akaIDIOT
Copy link
Contributor

@akaIDIOT akaIDIOT commented May 7, 2018

Includes commit by @cdavoren as his PR triggered this (changes effectively overwritten).

  • using datetime.timezone() for python3;
  • using minimal timezone() for python2.

Old behaviour:

>>> source = 'key: 2018-05-07T11:22:33+02:00'
>>> data = yaml.load(source)
>>> data['key']
datetime.datetime(2018, 5, 7, 9, 22, 33)

New behaviour:

>>> source = 'key: 2018-05-07T11:22:33+02:00'
>>> data = yaml.load(source)
>>> data['key']
datetime.datetime(2018, 5, 7, 11, 22, 33, tzinfo=datetime.timezone(datetime.timedelta(0, 7200)))
@K0Te
Copy link

@K0Te K0Te commented May 17, 2018

This change would be very helpful !

@akaIDIOT
Copy link
Contributor Author

@akaIDIOT akaIDIOT commented May 29, 2018

To be honest, I'm not at all happy with the implementation of timezone for the py2 sources. Any great ideas on how to do that better (short of depending on pytz, guessing the pyyaml team won't like adding deps)?

@seansfkelley
Copy link

@seansfkelley seansfkelley commented Apr 19, 2019

I just got bit by the lack of this behavior, so I'd love to see this PR get merged. What's wrong with the implementation as-is? If Python 2 doesn't have it, what else is feasible? I think you definitely don't want to add pytz.

Edit: @akaIDIOT you mentioned that you don't like the way this PR is written... what do you think of #251?

@akaIDIOT
Copy link
Contributor Author

@akaIDIOT akaIDIOT commented Apr 24, 2019

@seansfkelley they're basically the same thing I'd say. The timezone class is a little less messy than the one in this PR (and I personally hate dunder-private members :P), but I mainly care about the change in behaviour. The fact that multiple people are trying to fix this and others vote on the PR's tells me I'm not alone in this ;)

I'll clean up the timezone creation and rebase things on the current master, but it's up to the maintainers at this point.

@akaIDIOT
Copy link
Contributor Author

@akaIDIOT akaIDIOT commented Apr 24, 2019

Noticed the datetime regex matched a Z as timezone, also resulting in a naive, unshifted datetime. Made that use a zero-offset timezone for py2, datetime.timezone.utc for py3.

@seansfkelley
Copy link

@seansfkelley seansfkelley commented Apr 24, 2019

@ingydotnet could you take a look at this PR or #251? I've had to rewrap PyYAML/switch away from it in some cases due to its datetime behavior. :(

@jrd-nonlilly
Copy link

@jrd-nonlilly jrd-nonlilly commented Aug 5, 2019

It appears this problem has been around for the better part of a decade. Any hope of a merge?

@mattseymour
Copy link

@mattseymour mattseymour commented Sep 19, 2019

Is there an ETA on supporting TZ date times. Currently 163, 113 and 251 are all open which try to resolve this issue. It seems to be an issue being faced by a lot of people and the fact 3 pull requests are open for such an issue highlights this.

It would be good if someone from the core team could shed some light on why these changes have taken a while to get reviewed so we may assist where possible; whether it is that the devs are busy or have reservations about the PR's. In either case, it would be good to start a conversation around this so we can all contribute to help push this change into master (sooner rather than later).

Cross-posting to other tickets.

@perlpunk
Copy link
Member

@perlpunk perlpunk commented Dec 9, 2019

Thanks! I think it would be nice to add a test for this.
I'm trying to get this into v5.3 or v5.4

OTOH, stil not sure if #251 would be better

@akaIDIOT
Copy link
Contributor Author

@akaIDIOT akaIDIOT commented Dec 10, 2019

I'm fine with merging either, or editing this, I care most about the fix :)

I'll rebase the PR and clean it up a bit, see if I can add a test. Leave the actual choice up to the devs.

cdavoren and others added 8 commits Dec 10, 2019
On loading data, if timestamps have an ISO "+HH:MM" UTC offset then the resultant datetime is converted to UTC.  This change adds that timezone information to the datetime objects.

Importantly, this addresses a Django warning (and potential error) that appears when using both YAML fixtures in a timezone-aware project.  It was raised as a Django issue (https://code.djangoproject.com/ticket/18867), but subsequently closed because the Django devs felt that this is a PyYAML problem.
Call datetime.datetime constructor once at return.
Call datetime.datetime constructor once at return.
@akaIDIOT
Copy link
Contributor Author

@akaIDIOT akaIDIOT commented Dec 10, 2019

Test data already contains a bunch of datetimes with timezones that still seem to work. I'd be all for adding a test to make sure parsed values that have a timezone come out with a timezone attached, but the test code seems to want to just eval code and compare the values, not sure how easy it'd be to add a new assertion there somewhere...

@perlpunk
Copy link
Member

@perlpunk perlpunk commented Dec 15, 2019

I think I found a way to test for the timzones.
I will try and amend your PR...

edit: not sure that will work since your PR is made from master.
maybe I'll make my own PR after merging this

@perlpunk perlpunk changed the base branch from master to release/5.3 Dec 15, 2019
@perlpunk perlpunk merged commit 0cdef1d into yaml:release/5.3 Dec 15, 2019
2 checks passed
@perlpunk
Copy link
Member

@perlpunk perlpunk commented Dec 15, 2019

Thanks, merged!

@perlpunk perlpunk moved this from Backlog to Merged to release/5.3 in 5.3 Release Dec 15, 2019
perlpunk added a commit to perlpunk/pyyaml that referenced this issue Dec 15, 2019
After yaml#163, this adds some test data to check if the datetime objects
return the correct timezone
@perlpunk perlpunk mentioned this pull request Dec 15, 2019
@perlpunk
Copy link
Member

@perlpunk perlpunk commented Dec 15, 2019

@akaIDIOT I created #363 to add timezone tests. Maybe you could have a look if it makes sense?

perlpunk added a commit that referenced this issue Dec 16, 2019
After #163, this adds some test data to check if the datetime objects
return the correct timezone
perlpunk added a commit that referenced this issue Dec 16, 2019
After #163, this adds some test data to check if the datetime objects
return the correct timezone
perlpunk added a commit that referenced this issue Dec 20, 2019
* On load, now use aware datetimes if possible

On loading data, if timestamps have an ISO "+HH:MM" UTC offset then the resultant datetime is converted to UTC.  This change adds that timezone information to the datetime objects.

Importantly, this addresses a Django warning (and potential error) that appears when using both YAML fixtures in a timezone-aware project.  It was raised as a Django issue (https://code.djangoproject.com/ticket/18867), but subsequently closed because the Django devs felt that this is a PyYAML problem.

* Create timezone-aware datetime in timezone from data

* Create timezone-aware datetime in timezone from data for python2

* Define better timezone implementation for python2

* Handle timezone "Z" for python 3

* Handle timezone "Z" for python 2

* Fix code structure for Python 3

Call datetime.datetime constructor once at return.

* Fix code structure for Python 2

Call datetime.datetime constructor once at return.
perlpunk added a commit that referenced this issue Dec 20, 2019
After #163, this adds some test data to check if the datetime objects
return the correct timezone
@perlpunk
Copy link
Member

@perlpunk perlpunk commented Jan 6, 2020

asherf added a commit to asherf/pants that referenced this issue Apr 28, 2020
https://github.com/yaml/pyyaml/blob/d0d660d035905d9c49fc0f8dafb579d2cc68c0c8/CHANGES#L7

5.3.1 (2020-03-18)

* yaml/pyyaml#386 -- Prevents arbitrary code execution during python/object/new constructor

5.3 (2020-01-06)

* yaml/pyyaml#290 -- Use `is` instead of equality for comparing with `None`
* yaml/pyyaml#270 -- fix typos and stylistic nit
* yaml/pyyaml#309 -- Fix up small typo
* yaml/pyyaml#161 -- Fix handling of __slots__
* yaml/pyyaml#358 -- Allow calling add_multi_constructor with None
* yaml/pyyaml#285 -- Add use of safe_load() function in README
* yaml/pyyaml#351 -- Fix reader for Unicode code points over 0xFFFF
* yaml/pyyaml#360 -- Enable certain unicode tests when maxunicode not > 0xffff
* yaml/pyyaml#359 -- Use full_load in yaml-highlight example
* yaml/pyyaml#244 -- Document that PyYAML is implemented with Cython
* yaml/pyyaml#329 -- Fix for Python 3.10
* yaml/pyyaml#310 -- increase size of index, line, and column fields
* yaml/pyyaml#260 -- remove some unused imports
* yaml/pyyaml#163 -- Create timezone-aware datetimes when parsed as such
* yaml/pyyaml#363 -- Add tests for timezone

5.2 (2019-12-02)
------------------

* Repair incompatibilities introduced with 5.1. The default Loader was changed,
  but several methods like add_constructor still used the old default
  yaml/pyyaml#279 -- A more flexible fix for custom tag constructors
  yaml/pyyaml#287 -- Change default loader for yaml.add_constructor
  yaml/pyyaml#305 -- Change default loader for add_implicit_resolver, add_path_resolver
* Make FullLoader safer by removing python/object/apply from the default FullLoader
  yaml/pyyaml#347 -- Move constructor for object/apply to UnsafeConstructor
* Fix bug introduced in 5.1 where quoting went wrong on systems with sys.maxunicode <= 0xffff
  yaml/pyyaml#276 -- Fix logic for quoting special characters
* Other PRs:
  yaml/pyyaml#280 -- Update CHANGES for 5.1
asherf added a commit to asherf/pants that referenced this issue Apr 29, 2020
https://github.com/yaml/pyyaml/blob/d0d660d035905d9c49fc0f8dafb579d2cc68c0c8/CHANGES#L7

5.3.1 (2020-03-18)

* yaml/pyyaml#386 -- Prevents arbitrary code execution during python/object/new constructor

5.3 (2020-01-06)

* yaml/pyyaml#290 -- Use `is` instead of equality for comparing with `None`
* yaml/pyyaml#270 -- fix typos and stylistic nit
* yaml/pyyaml#309 -- Fix up small typo
* yaml/pyyaml#161 -- Fix handling of __slots__
* yaml/pyyaml#358 -- Allow calling add_multi_constructor with None
* yaml/pyyaml#285 -- Add use of safe_load() function in README
* yaml/pyyaml#351 -- Fix reader for Unicode code points over 0xFFFF
* yaml/pyyaml#360 -- Enable certain unicode tests when maxunicode not > 0xffff
* yaml/pyyaml#359 -- Use full_load in yaml-highlight example
* yaml/pyyaml#244 -- Document that PyYAML is implemented with Cython
* yaml/pyyaml#329 -- Fix for Python 3.10
* yaml/pyyaml#310 -- increase size of index, line, and column fields
* yaml/pyyaml#260 -- remove some unused imports
* yaml/pyyaml#163 -- Create timezone-aware datetimes when parsed as such
* yaml/pyyaml#363 -- Add tests for timezone

5.2 (2019-12-02)
------------------

* Repair incompatibilities introduced with 5.1. The default Loader was changed,
  but several methods like add_constructor still used the old default
  yaml/pyyaml#279 -- A more flexible fix for custom tag constructors
  yaml/pyyaml#287 -- Change default loader for yaml.add_constructor
  yaml/pyyaml#305 -- Change default loader for add_implicit_resolver, add_path_resolver
* Make FullLoader safer by removing python/object/apply from the default FullLoader
  yaml/pyyaml#347 -- Move constructor for object/apply to UnsafeConstructor
* Fix bug introduced in 5.1 where quoting went wrong on systems with sys.maxunicode <= 0xffff
  yaml/pyyaml#276 -- Fix logic for quoting special characters
* Other PRs:
  yaml/pyyaml#280 -- Update CHANGES for 5.1
Eric-Arellano pushed a commit to pantsbuild/pants that referenced this issue May 1, 2020
https://github.com/yaml/pyyaml/blob/d0d660d035905d9c49fc0f8dafb579d2cc68c0c8/CHANGES#L7

5.3.1 (2020-03-18)

* yaml/pyyaml#386 -- Prevents arbitrary code execution during python/object/new constructor

5.3 (2020-01-06)

* yaml/pyyaml#290 -- Use `is` instead of equality for comparing with `None`
* yaml/pyyaml#270 -- fix typos and stylistic nit
* yaml/pyyaml#309 -- Fix up small typo
* yaml/pyyaml#161 -- Fix handling of __slots__
* yaml/pyyaml#358 -- Allow calling add_multi_constructor with None
* yaml/pyyaml#285 -- Add use of safe_load() function in README
* yaml/pyyaml#351 -- Fix reader for Unicode code points over 0xFFFF
* yaml/pyyaml#360 -- Enable certain unicode tests when maxunicode not > 0xffff
* yaml/pyyaml#359 -- Use full_load in yaml-highlight example
* yaml/pyyaml#244 -- Document that PyYAML is implemented with Cython
* yaml/pyyaml#329 -- Fix for Python 3.10
* yaml/pyyaml#310 -- increase size of index, line, and column fields
* yaml/pyyaml#260 -- remove some unused imports
* yaml/pyyaml#163 -- Create timezone-aware datetimes when parsed as such
* yaml/pyyaml#363 -- Add tests for timezone

5.2 (2019-12-02)
------------------

* Repair incompatibilities introduced with 5.1. The default Loader was changed,
  but several methods like add_constructor still used the old default
  yaml/pyyaml#279 -- A more flexible fix for custom tag constructors
  yaml/pyyaml#287 -- Change default loader for yaml.add_constructor
  yaml/pyyaml#305 -- Change default loader for add_implicit_resolver, add_path_resolver
* Make FullLoader safer by removing python/object/apply from the default FullLoader
  yaml/pyyaml#347 -- Move constructor for object/apply to UnsafeConstructor
* Fix bug introduced in 5.1 where quoting went wrong on systems with sys.maxunicode <= 0xffff
  yaml/pyyaml#276 -- Fix logic for quoting special characters
* Other PRs:
  yaml/pyyaml#280 -- Update CHANGES for 5.1
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
5.3 Release
Merged to release/5.3
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants