Incorrect tzinfo check in tslibs.parsing

[jbrockmendel]

https://github.com/pandas-dev/pandas/blob/master/pandas/_libs/tslibs/parsing.pyx#L339

    if isinstance(tzdata, datetime.tzinfo):
                tzinfo = tzdata

If this were ever hit it would raise an error because datetime.tzinfo refers to the tzinfo attached to a datetime object, not the tzinfo class defined in the datetime module.

There's basically no path by which this would be reached because we never pass a tzdata to this function. So two things should happen: fix this to check for the correct object, and either remove unreachable code or make it reachable.

[mroeschke]

The same check (bug?) exists in dateutil:

https://github.com/dateutil/dateutil/blob/99f5770e7c63aa049b28abe465d7f1cc25b63fd2/dateutil/parser/_parser.py#L1161

Also it looks like tzdata is defined right above this check in the if/else above.

However if there are no conditions where we ever hit this branch, I would vote nix it.

[jbrockmendel]

The same check (bug?) exists in dateutil:

dateutil.parser._parser has import datetime, whereas tslibs.parsing has from cpython.datetime cimport datetime (roughly equivalent to from datetime import datetime)

[mroeschke]

Ah good catch. Looks like a copy-paste bug then.

But I think this branch will never be hit:

So dateutil_parse where this code lies in only called in one place

pandas/pandas/_libs/tslibs/parsing.pyx

Lines 159 to 160 in 737b329

	parsed, reso = dateutil_parse(date_string, _DEFAULT_DATETIME,
	dayfirst=dayfirst, yearfirst=yearfirst)

And a default kwarg tzinfos will therefore always be None and this branch where this code lies will always be False:

pandas/pandas/_libs/tslibs/parsing.pyx

Line 334 in 737b329

if callable(tzinfos) or tzinfos and res.tzname in tzinfos:

[gfyoung]

I would just remove the logic and see if any tests break. That would help to confirm 🙂

[jbrockmendel]

However if there are no conditions where we ever hit this branch, I would vote nix it.

We briefly discussed at the sprint the idea of correctly parsing some timezone strings where the correct answer is unambiguous (e.g. "CEST" should always be "Central European Summer Time"). If we go down that road, it will be by passing tzinfos as a dict.

i.e. we shouldn't nix all of that code, and as long as we have some of it, it's worth keeping it parallel to the dateutil version (since it is basically copy/pasted)

[mroeschke]

I see. In that case, makes sense to keep it for now.

To pay it forward though we can at least fix the datetime.tzinfo check and add some comments why that block is not getting hit (and what it will be useful for in the future).