closeio / ciso8601

Fast ISO8601 date time parser for Python written in C
MIT License
565 stars 45 forks source link

Better error handling if pytz is missing #19

Closed thomasst closed 6 years ago

thomasst commented 7 years ago

See https://github.com/closeio/ciso8601/issues/18

We should probably do a better job at failing in https://github.com/closeio/ciso8601/blob/master/module.c#L245

I think right now it leaves pytz_fixed_offset and pytz_utc uninitialized, which is bad. (it's a global, so it's NULL by default)

prymitive commented 7 years ago

Would you consider adding pytz as a global dependency? Is there any performance penalty when pytz is installed but timestamps do not have timezone information? Or maybe simply add a warning near the Usage section of the README file?

thomasst commented 7 years ago

pytz is only required if we want to parse aware-datetimes, so ideally I'd prefer to keep pytz optional. We could only expose parse_datetime_unaware if pytz is not installed. Alternatively, we could fail (or ignore the timezone) in parse_datetime, but I'd probably prefer failing early.

batkuip commented 6 years ago

Currently, it's parsing but ignoring timezone information if pytz is not available. So you get a valid date back, but it's just wrong. This was a nasty bug to track down. On one system it was working fine, the other it wasn't with the same dates. Thought I was going mental.

dahlia commented 6 years ago

We could handle conditional dependencies using environment markers. It's basically like this:

setup(
    ...,
    extras_requires={
        ":python_version<'3.0'": ['pytz'],
    }
)
peterbe commented 6 years ago

It took me totally by surprise

I am personally lukewarm on depending on pytz. I can see how it's nice to be able to avoid it if you know you don't need aware datetimes. But then I think only parse_datetime_unaware should work. If you use parse_datetime without pytz installed I think you should get an exception raised. My 2c.

peterbe commented 6 years ago

By the way, I blogged this time today: https://www.peterbe.com/plog/fastest-python-datetime-parser

movermeyer commented 6 years ago

@dahlia If ciso8601 was written at the Python level (and not the C-API level), then we could make use of datetime.timezone and drop the dependency on pytz for Python 3.2+.

However, datetime.timezone objects are not exposed to the C-API until Python 3.7 :cry: . So ciso8601 still requires pytz for all current versions of Python.

I will make the change to have it raise an ImportError if someone tries to parse an aware timestamp with parse_datetime()

thomasst commented 6 years ago

Would be nice if we could use the timezone C API starting with Python 3.7.

movermeyer commented 6 years ago

@thomasst, Already working on it :wink:

movermeyer commented 6 years ago

Actually, we can make use of datetime.timezone for Python [3.2, 3.6], but only by accessing it through the Python API (not the C API that was added in 3.7). This still provides a nice performance boost over using pytz, so @dahlia is correct after all. We could drop our pytz dependency for Python 3. :smiley:

movermeyer commented 6 years ago

Fixed in version 2.0.0 (#43).

It now only tries to use pytz in Python 2, and will throw an exception if there is timezone information in the provided timestamp but pytz is not installed.