Warn on unidentified tz by jbrockmendel · Pull Request #540 · dateutil/dateutil · GitHub

Current incorrect behavior:

>>> parser.parse('2017-12-05 08:38 EST')
datetime.datetime(2017, 12, 5, 8, 38)

This PR raises instead of silently dropping this timezone information.

This surfaces two test failures that look like false-positives slipped through and got assigned to res.tzname. Thats the "WIP" part of the PR.

___________________________________________ ParserTest.testFuzzyIgnoreAMPM ___________________________________________
[...]
timestr = 'Jan 29, 1945 14:45 AM I going to see you there?'
res = _result(year=1945, month=1, day=29, hour=14, minute=45, tzname=u'I')

___________________________________________ ParserTest.testRandomFormat26 ____________________________________________
[...]
timestr = '5:50 A.M. on June 13, 1990'
_result(year=1990, month=6, day=13, hour=5, minute=50, tzname=u'M', ampm=0)

Broadly speaking I'm in favor of this, but I'll note that the behavior of datetime.strptime's %Z directive is to silently drop time zones, weirdly. I think we will want to have a workaround in place to get the old behavior back quickly.

Maybe some sort of tzinfos wrapper object that silently drops any unrecognized time zones?

I haven't given any thought to the strptime implications. For the moment I'm planning to focus on the two errors this turned up.

Maybe some sort of tzinfos wrapper object that silently drops any unrecognized time zones?

Do you mean an option that allows users to restore the current behavior?

I haven't given any thought to the strptime implications. For the moment I'm planning to focus on the two errors this turned up.

It really has no implications for strptime, it's just that that may indicate that people are expecting this kind of behavior (or that both datetime and dateutil made the same unfortunate and surprising choice). It should probably give us pause, though, since we don't know how many people are relying on this behavior.

Do you mean an option that allows users to restore the current behavior?

Yes, though I wasn't thinking of a keyword argument. Maybe it's too abstruse, but I was thinking of something roughly like this (this is for the case that tzinfos is a dict, though we'd have to handle all cases).

def tzinfoswrapper(tzinfos, drop_missing=True):
    return tzinfos.get if drop_missing else tzinfos

That will return a tzinfo for any specified zones and None for anything else (silently dropping the time zone). Then you could pass tzinfoswrapper(tzinfos) instead of tzinfos, and you'll get the old behavior. Either way, we need an easy way to do this.

The tzinfoswrapper idea could be another shortcuttable-default in the same vein as #575.

I think the correct order to do things is to merge #554 (or another equivalent fix to that problem), then this.

Just changed raise to a warning per brief discussion in #541.

We should raise a specific warning type so that it can be easily ignored by people who don't want to see this message. I suggest subclassing maybe RuntimeWarning, maybe UnknownTimezoneWarning(RuntimeWarning)?

Also, I wouldn't specify ValueError in the message. Just say it will raise an error.

Also, I wouldn't specify ValueError in the message. Just say it will raise an error.

But the error (in the future) will be a ValueError (or a subclass), so the users seeing it can already prepare for it.

@wagner-certat It might be a ValueError, but probably it will be a subclass. I'm not quite sure how they would "prepare for it" using just the information that it might raise a ValueError, though, since that would catch nearly all the errors raised by parse.

True, I also catch all ValueErrors already now.

Hm, I guess I didn't notice this in the first round of review. This is one of those xfail-ish tests I guess.

Yah, I think this is part of what motivated the PR about checking for 1-character timezones more carefully (and from there discussion about keeping "A.M." in one piece in timelex.split...). Hopefully we'll be able to get rid of this line before long.

@jbrockmendel Is this still WIP or is it ready to merge?

Is this still WIP or is it ready to merge?

I haven't implemented the suggestion to make a specific subclass UnknownTimezoneWarning. If you're OK leaving that for a follow-up, then I think this is good to go.

@jbrockmendel I'll wait for now, but if it gets close to release time (which should be soon-ish) if it's not done I'll just add that myself.

@jbrockmendel At this point we're just blocking on this, #613 and #571 for the 2.7.0 release. I think I might push #571 to 2.7.1 just to get this ridiculous number of changes out the door. Like I said, I can make the required changes right before merge, but just pinging in case you've forgotten about this one.

This has fallen off my radar screen, will likely stay that way for a while longer. I'll make a note to circle back Feb 1, but feel free to run with it if that's too long a wait.

@jbrockmendel Planning on cutting a release today, do you want to review my changes?

I'll take a look.

edit warning per suggestion

OK, this is the last blocker, so as soon as the rebased version passes CI I'm going to merge.

LGTM

While #575 is possibly in development, are there any plans to add the tzinfoswrapper idea or another easy way to restore the old behavior?
I'd rather not have to temporarily filter/suppress the warning until I can catch an exception in a future version.

@Harmon758 Your comment made me realize that there actually isn't a good way to get the old behavior, so I've opened #661 as a first pass.

The warning is a specific warning class so that it should be easy to filter out if you don't care about it, so for now filtering out the behavior is the way to get the old behavior. There will be a clear way to opt in to the old behavior before the final switch happens.

pganssle added enhancement parser labels Dec 5, 2017

pganssle added this to the Feature release milestone Dec 5, 2017

jbrockmendel mentioned this pull request Dec 6, 2017

fix bug with incorrectly picking out 1-character tznames #554

Open

This was referenced Dec 8, 2017

Sensible defaults for tzinfos #575

Closed

Timezones silently dropped in parsing pandas-dev/pandas#18702

Closed

Prepocessing results of timelex.split #541

Closed

pganssle reviewed Dec 13, 2017

View reviewed changes

pganssle modified the milestones: Feature release, 2.7.0 Dec 15, 2017

pganssle reviewed Dec 18, 2017

View reviewed changes

pganssle changed the title WIP: raise on unidentified tz Warn on unidentified tz Mar 11, 2018

pganssle force-pushed the catch_tzs branch from 61fa2be to 4ba61c6 Compare March 11, 2018 19:48

pganssle mentioned this pull request Mar 11, 2018

Release 2.7.0 #627

Closed

jbrockmendel and others added 3 commits March 11, 2018 18:15

raise on unidentified tz

1dfa77a

warn instead of raising... for now

8796170

edit warning per suggestion

Switch unknown timezone warning type

7b242e2

pganssle force-pushed the catch_tzs branch from 4ba61c6 to 7b242e2 Compare March 11, 2018 22:16

pganssle merged commit 65d66b2 into dateutil:master Mar 11, 2018

pganssle mentioned this pull request Apr 8, 2018

Allow tzinfos to return None #661

Closed

pganssle mentioned this pull request Apr 9, 2018

No error raised on invalid dates arrow-py/arrow#527

Closed

jbrockmendel deleted the catch_tzs branch December 29, 2019 18:18

jbrockmendel mentioned this pull request Dec 29, 2019

'parse()' method converting junk value to date #988

Open

                   def testRandomFormat26(self):
-                      self.assertEqual(parse("5:50 A.M. on June 13, 1990"),
-                                       datetime(1990, 6, 13, 5, 50))
+                      with pytest.warns(UserWarning, match="identified but not understood"):

Warn on unidentified tz by jbrockmendel · Pull Request #540 · dateutil/dateutil · GitHub

Warn on unidentified tz - #540

Warn on unidentified tz#540pganssle merged 3 commits intodateutil:masterdateutil/dateutil:masterfrom jbrockmendel:catch_tzsjbrockmendel/dateutil:catch_tzsCopy head branch name to clipboard

Conversation

jbrockmendel commented Dec 5, 2017

Uh oh!

pganssle commented Dec 5, 2017

Uh oh!

jbrockmendel commented Dec 5, 2017

Uh oh!

pganssle commented Dec 5, 2017

Uh oh!

jbrockmendel commented Dec 8, 2017

Uh oh!

jbrockmendel commented Dec 13, 2017

Uh oh!

pganssle Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

pganssle Dec 13, 2017

Choose a reason for hiding this comment

Uh oh!

ghost Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

pganssle Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

ghost Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

pganssle Dec 18, 2017

Choose a reason for hiding this comment

Uh oh!

jbrockmendel Dec 19, 2017

Choose a reason for hiding this comment

Uh oh!

pganssle commented Dec 19, 2017

Uh oh!

jbrockmendel commented Dec 19, 2017

Uh oh!

pganssle commented Dec 20, 2017

Uh oh!

pganssle commented Jan 18, 2018

Uh oh!

jbrockmendel commented Jan 18, 2018

Uh oh!

pganssle commented Mar 11, 2018

Uh oh!

jbrockmendel commented Mar 11, 2018

Uh oh!

pganssle commented Mar 11, 2018

Uh oh!

jbrockmendel commented Mar 11, 2018

Uh oh!

Harmon758 commented Apr 7, 2018

Uh oh!

pganssle commented Apr 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Warn on unidentified tz#540
pganssle merged 3 commits into
dateutil:masterfrom
jbrockmendel:catch_tzs