London Sprint - imaginaryDateTest #565

Cheukting · 2017-12-06T22:17:12Z

London Sprint #339

Pushing function resolve_imaginary in tz, created 4 test cases which 3 of them pass. the only one that fail is testMonroviaForward which is dealing with + 44mins 30seconds time zone changes. Further testing suggest that utcoffset ignore the 30seconds so it 0:30 on 7 Jan 1972 will become 1:14, not 1:14:30 on the same day.

jbrockmendel · 2017-12-06T22:55:05Z

.cache/v/cache/lastfailed

+  "dateutil/test/test_tz.py::TzWinTest::testTzwinName": true,
+  "dateutil/test/test_tz.py::imaginaryDateTest::testMonroviaForward": true,
+  "dateutil/test/test_utils.py": true
+}


These cache files generally don’t belong in the commit. Is this here intentionally?

I think they are automatically generated during testing, can be deleted. I will have to tidy things up and push it again.

jbrockmendel · 2017-12-06T22:56:09Z

dateutil/tz/tz.py

        pass

+def resolve_imaginary(dt):
+    import pdb;pdb.set_trace()


Is the Pdb needed outside of testing?

Sorry I was finishing in a rush, should be for testing only, I can tidy it up and push it again.

pganssle · 2017-12-11T14:01:23Z

@Cheukting FYI, it's generally not a great idea to work in the master branch of your fork. It's fine in this case, but it will make it harder to work in the future.

Cheukting · 2017-12-11T15:01:33Z

Thanks @pganssle for the tip. I am new in term of developing packages so still need to learn what's the best practice. Will create a new branch next time. 😃

pganssle

First of all, I'd like to say thank you so much for your contribution. Even if you make no further changes to this PR, I am happy to clone your branch and make the changes I'd like before merging.

I have made a few stylistic notes about this PR - these are basically nitpicks, but I feel that it would be doing you a disservice if I did not give you feedback about the preferred style (since that's the way you find out what people expect). As I mention above, I'm happy to fix all the nitpicks myself.

You also found a new bug in dateutil (#582)! So that's a good thing!

One other thing is that there are a few more tests to add:

Add a test with a 1-hour offset that takes place in the northern hemisphere.
Add a test for Europe/London (which is an unusual zone).
Add a test for something in the eastern hemisphere (maybe Europe/Kiev)
Add two tests that this returns the same object during an ambiguous time - one in the northern (America/New_York maybe) and one in the southern hemisphere (Sydney is fine).
Add a test that returns the same object during a non-ambiguous time in the northern hemisphere.
Add a test that this returns the same object when handed a date already in UTC.

To summarize the changes to be made before this PR can be merged (and keep in mind any of the ones you don't do I can finish up):

Remove the .cache files.
Add a docstring.
Clean up the style of the tests
Add the additional tests detailed above

pganssle · 2017-12-11T14:09:19Z

dateutil/test/test_tz.py

        # Before Python 3.6, dt.fold won't exist if fold is 0.
        self.assertEqual(getattr(dt, 'fold', 0), 0)
+
+class imaginaryDateTest(unittest.TestCase):


If you're going to do unittest style tests, the classes use CapWords, not camelCase, so this should be ImaginaryDateTest (the methods - the test names themselves - are fine as camelCase).

pganssle · 2017-12-11T14:09:57Z

dateutil/test/test_tz.py

+        timezone = tz.gettz('Australia/Canberra')
+        dt = datetime(2018, 10, 7, 2, 30, tzinfo=timezone)
+        dt = tz.resolve_imaginary(dt)
+        self.assertEqual(dt,datetime(2018, 10, 7, 3, 30, tzinfo=timezone))


There should be a space between the comma and datetime here. I would also probably not reassign dt here, but that's a minor thing.

pganssle · 2017-12-11T14:11:10Z

dateutil/test/test_tz.py

+
+class imaginaryDateTest(unittest.TestCase):
+    def testCanberraForward(self):
+        timezone = tz.gettz('Australia/Canberra')


timezone is actually already the name of a class in datetime in Python 3.3. I'd rather not shadow that variable, even though it causes no problems in this case. Probably call it tzi or something like CANBERRA.

pganssle · 2017-12-11T14:12:45Z

dateutil/test/test_tz.py

+        timezone = tz.gettz('Australia/Sydney')
+        dt = datetime(2018, 4, 1, 3, 30, tzinfo=timezone)
+        dtnew = tz.resolve_imaginary(dt)
+        self.assertEqual(dt,dtnew)


I would like the guarantee here to be even stronger than assertEqual - this should in fact return the exact same object, so this should be changed to self.assertIs.

pganssle · 2017-12-11T14:13:55Z

dateutil/tz/tz.py

    def __exit__(*args, **kwargs):
        pass

+def resolve_imaginary(dt):


As this is a public function, it needs a docstring, explaining what it does, etc. See enfold, datetime_exists and datetime_ambiguous as examples.

pganssle · 2017-12-11T14:24:31Z

dateutil/test/test_tz.py

+        timezone = tz.gettz('Africa/Monrovia')
+        dt = datetime(1972, 1, 7, hour = 0, minute = 30, second = 0, tzinfo=timezone)
+        dt = tz.resolve_imaginary(dt)
+        self.assertEqual(dt,datetime(1972, 1, 7, hour = 1, minute =  14,  second = 30, tzinfo=timezone))


This is the failing test. I believe the reason it's failing is that tz.tzfile has the wrong behavior and is rounding time zones down to the nearest minute. Before Python 3.6, this was necessary because Python did not support non-integer number of minute offsets, but that is changed now. I'll make a PR that fixes this issue and that will allow this test to pass.

Another style note about this function - PEP8 recommends no space between keywords and arguments in functions. As a demonstration, here is how I would write this same function, taking into account the other stylistic notes from above:

def testMonroviaForward(self): tzi = tz.gettz('Africa/Monrovia') dt = datetime(1972, 1, 7, hour=0, minute=30, second=0, tzinfo=tzi) dt_act = tz.resolve_imaginary(dt) dt_exp = datetime(1972, 1, 7, hour=1, minute=14, second=30, tzinfo=tzi) self.assertEqual(dt, dt_exp)

pganssle · 2017-12-11T15:19:35Z

dateutil/test/test_tz.py

+        dt = tz.resolve_imaginary(dt)
+        self.assertEqual(dt,datetime(2018, 10, 7, 3, 30, tzinfo=timezone))
+
+    def testSydneyBack(self):


I didn't realize this originally, but datetime(2018, 4, 1, 3, 30) is very close to, but not actually an ambiguous time. It might be best to use a more obviously unambiguous time like datetime(2018, 6, 2, 12, 0).

Also maybe rename this to be like testSydneyExisting and/or add a comment like # Tests that resolve_imaginary has no effect on normal datetimes.

pganssle · 2017-12-12T13:32:28Z

@Cheukting No matter what you end up doing, can you please add yourself to the AUTHORS.md file? Even if you make no other changes, your work on this so far deserves recognition.

pganssle · 2018-01-03T23:05:07Z

Consolidated into #607.

London Sprint - imaginaryDateTest

890a304

jbrockmendel reviewed Dec 6, 2017

View reviewed changes

London Sprint - imarginary date test

15ad9af

pganssle added tests time zones labels Dec 7, 2017

pganssle added this to the 2.7.0 milestone Dec 7, 2017

pganssle mentioned this pull request Dec 11, 2017

tzfile does not support non-integer minute offsets #582

Closed

pganssle requested changes Dec 11, 2017

View reviewed changes

pganssle reviewed Dec 11, 2017

View reviewed changes

Cheukting mentioned this pull request Dec 18, 2017

Imaginary date test same as #565 #593

Closed

pganssle mentioned this pull request Jan 3, 2018

Add resolve_imaginary #607

Merged

pganssle closed this Jan 3, 2018

London Sprint - imaginaryDateTest #565

London Sprint - imaginaryDateTest #565

Uh oh!

Conversation

Cheukting commented Dec 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

pganssle commented Dec 11, 2017

Uh oh!

Cheukting commented Dec 11, 2017

Uh oh!

pganssle left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

pganssle commented Dec 12, 2017

Uh oh!

pganssle commented Jan 3, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Cheukting commented Dec 6, 2017 •

edited

Loading

pganssle left a comment •

edited

Loading