Closes #582: Allow partial minutes for python 3.6 or newer by cssherry · Pull Request #763 · dateutil/dateutil

cssherry · 2018-06-08T16:37:41Z

Summary of changes

As described in python bug tracker (https://bugs.python.org/issue5288), datetime now allows offsets that are not an integer number of minutes. Dateutil now accounts for this when running in newer versions of python

Add "Africa/Monrovia" to test_resolve_imaginary test for python 3.6 or newer
Test that partial minutes is supported in python 3.6 or newer for tz.tzfile and datetime.utcoffset
Enable partial minutes in both tz.tzoffset and tz.tzfile
For now, don't warn user when rounding to nearest minute for python pre-3.6

Closes #582

Pull Request Checklist

Changes have tests
Authors have been added to AUTHORS.md
News fragment added in changelog.d. See CONTRIBUTING.md for details

pganssle · 2018-06-08T17:05:49Z

    return (dt.replace(tzinfo=None) - EPOCH).total_seconds()


+def supports_seconds():


I would not like to change the public API at all with this, there should be no way to query this.

Also, this has a runtime cost when I would prefer an import-time cost.

fixed, made private and calculate correct function at import

pganssle · 2018-06-08T17:07:04Z

-        self.assertEqual(str(datetime(1900, 1, 1, 0, 0, tzinfo=tzc)),
-                             "1900-01-01 00:00:00+01:40")
+        # For python version pre-3.6, this will be rounded
+        set_as_old_python()


There's no need to mock out the old behavior - we don't need to verify that this behavior is accurate on all versions of Python. It's best to split this into two tests, one for pre-3.6 and one for 3.6+, and then use pytest.skipif to run them selectively based on version.

pganssle · 2018-06-09T07:37:36Z

+    def testPartialMinuteSupport(self):
+        # If user running python 3.6 or newer, exact offset is used
+        tzc = tz.tzfile(BytesIO(base64.b64decode(EUROPE_HELSINKI)))
+        self.assertEqual(str(datetime(1900, 1, 1, 0, 0, tzinfo=tzc)),


I think I would prefer a test that looks like this:

assert datetime(1900, 1, 1, 0, 0, tzinfo=tzc).utcoffset() == timedelta(hours=1, minutes=39, seconds=52)

Though I am a bit torn because I will admit that the "compare the string" version sure is easy to read.

The original was with string (see testRoundNonFullMinutes), but I can change them both to timedelta.

However, note that the two would test slightly different things (assert datetime(1900, 1, 1, 0, 0, tzinfo=tzc).utcoffset() == timedelta(hours=1, minutes=39, seconds=52) tests the date offset through utcoffset function, while the current version tests just the date formation from specified offset)

I'll make the change, but in separate commit for easy undo in case we decide to go with original method

pganssle · 2018-06-09T07:38:07Z

+        self.assertEqual(str(datetime(1900, 1, 1, 0, 0, tzinfo=tzc)),
+                         "1900-01-01 00:00:00+01:39:52")
+
+        delta = timedelta(hours=12, seconds=30)


I would prefer this to be a separate test (or better yet, a parametrized test).

sure, made this and second part of testRoundNonFullMinutes separate tests

cssherry

I noticed there's also offset in tzlocal class and tzfile.fromutc function. Not sure where/if that needs to get trimmed, but haven't touched them

cssherry · 2018-06-08T16:49:36Z

-    assert dt_r == dt_exp
-    assert dt_r.tzname() == dt_exp.tzname()
-    assert dt_r.utcoffset() == dt_exp.utcoffset()
+    if tz.supports_seconds() or tzi != tz.gettz('Africa/Monrovia'):


Alternatively, just remove Africa/Monrovia test from this set of tests

cssherry · 2018-06-08T16:54:30Z

+           sys.version_info[0] > 3
+
+
+def get_supported_offset(offset):


There's also offset in tzlocal class. Not sure where/if that needs to get trimmed

cssherry · 2018-06-08T22:04:12Z

-        self.assertEqual(str(datetime(1900, 1, 1, 0, 0, tzinfo=tzc)),
-                             "1900-01-01 00:00:00+01:40")
+        # For python version pre-3.6, this will be rounded
+        set_as_old_python()


cssherry · 2018-06-08T22:04:46Z

    return (dt.replace(tzinfo=None) - EPOCH).total_seconds()


+def supports_seconds():


fixed, made private and calculate correct function at import

cssherry · 2018-06-10T13:01:18Z

+        self.assertEqual(str(datetime(1900, 1, 1, 0, 0, tzinfo=tzc)),
+                         "1900-01-01 00:00:00+01:39:52")
+
+        delta = timedelta(hours=12, seconds=30)


sure, made this and second part of testRoundNonFullMinutes separate tests

cssherry · 2018-06-10T13:01:52Z

+    def testPartialMinuteSupport(self):
+        # If user running python 3.6 or newer, exact offset is used
+        tzc = tz.tzfile(BytesIO(base64.b64decode(EUROPE_HELSINKI)))
+        self.assertEqual(str(datetime(1900, 1, 1, 0, 0, tzinfo=tzc)),


The original was with string (see testRoundNonFullMinutes), but I can change them both to timedelta.

However, note that the two would test slightly different things (assert datetime(1900, 1, 1, 0, 0, tzinfo=tzc).utcoffset() == timedelta(hours=1, minutes=39, seconds=52) tests the date offset through utcoffset function, while the current version tests just the date formation from specified offset)

cssherry · 2018-06-10T13:07:18Z

+    def testPartialMinuteSupport(self):
+        # If user running python 3.6 or newer, exact offset is used
+        tzc = tz.tzfile(BytesIO(base64.b64decode(EUROPE_HELSINKI)))
+        self.assertEqual(str(datetime(1900, 1, 1, 0, 0, tzinfo=tzc)),


I'll make the change, but in separate commit for easy undo in case we decide to go with original method

pganssle · 2018-06-12T15:35:22Z

        tzc = tz.tzfile(BytesIO(base64.b64decode(EUROPE_HELSINKI)))
-        self.assertEqual(str(datetime(1900, 1, 1, 0, 0, tzinfo=tzc)),
-                             "1900-01-01 00:00:00+01:40")
+        assert datetime(1900, 1, 1, 0, 0, tzinfo=tzc).utcoffset(), timedelta(hours=1, minutes=40)


I think this needs to be:

assert datetime(1900, 1, 1, 0, 0, tzinfo=tzc).utcoffset() == timedelta(hours=1, minutes=40)

- Add "Africa/Monrovia" to test_resolve_imaginary test for python 3.6 or newer - Test that partial minutes is supported in python 3.6 or newer for tz.tzfile and datetime.utcoffset - Enable partial minutes in both tz.tzoffset and tz.tzfile - For now, don't warn user when rounding to nearest minute for python pre-3.6

cssherry force-pushed the partial_minutes_582 branch from 630a6fe to 091c496 Compare June 8, 2018 16:38

pganssle added the time zones label Jun 8, 2018

cssherry force-pushed the partial_minutes_582 branch 3 times, most recently from 8d88191 to d2600d8 Compare June 8, 2018 16:56

pganssle reviewed Jun 8, 2018

View reviewed changes

cssherry force-pushed the partial_minutes_582 branch 2 times, most recently from b81d332 to 6413db4 Compare June 8, 2018 23:29

pganssle reviewed Jun 9, 2018

View reviewed changes

cssherry force-pushed the partial_minutes_582 branch from dbc0a17 to 29605a0 Compare June 10, 2018 13:26

cssherry commented Jun 10, 2018

View reviewed changes

danchr mentioned this pull request Jun 11, 2018

ISO parser fixes to seconds & minutes #766

Open

3 tasks

pganssle reviewed Jun 12, 2018

View reviewed changes

cssherry force-pushed the partial_minutes_582 branch 2 times, most recently from 0ed3eb6 to d676718 Compare June 18, 2018 21:18

pganssle force-pushed the partial_minutes_582 branch from d676718 to 44e4a6c Compare June 18, 2018 22:54

cssherry and others added 2 commits June 18, 2018 19:09

Switch sub-minute offset tests to use pytest conventions

41e5176

pganssle force-pushed the partial_minutes_582 branch from 44e4a6c to 41e5176 Compare June 18, 2018 23:09

pganssle merged commit 6dde5d6 into dateutil:master Jun 18, 2018

pganssle mentioned this pull request Feb 9, 2019

tz produces strange timezone for datetime.isoformat() #866

Closed

		return (dt.replace(tzinfo=None) - EPOCH).total_seconds()


		def supports_seconds():

Conversation

cssherry commented Jun 8, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Pull Request Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cssherry left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cssherry commented Jun 8, 2018 •

edited

Loading