Loader.load portability broken for some pytz installs

[sdispater]

Loader.load portability broken for some pytz installs

[sdispater]

Yeah. We should deprecate one or the other.

I think the best approach here is just to remove Parser and move its methods to the loader class, since the name Parser is not exactly clear about what its purpose is.

[sdispater]

That's odd. What is the output of the following?

>>> import pytz
>>> import os
>>> import inspect

>>> os.path.dirname(inspect.getfile(pytz))

More importantly, does it return /usr/lib/python3.5/site-packages/pytz 

[mayfield]

/usr/lib/python3.5/site-packages/pytz

Which is there, it just doesn't have a zoneinfo dir.

[mayfield]

So in my install the zoneinfo files are shared with the system...

>>> import pytz
>>> pytz.open_resource('adsf')
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/lib/python3.5/site-packages/pytz/__init__.py", line 90, in open_resource
    return open(filename, 'rb')
FileNotFoundError: [Errno 2] No such file or directory: '/usr/share/zoneinfo/adsf'

That open_resource function from pytz might be your huckleberry.

[sdispater]

Which version of pytz do you have?

[sdispater]

Here is the open_resource function from pytz:

def open_resource(name):
    """Open a resource from the zoneinfo subdir for reading.

    Uses the pkg_resources module if available and no standard file
    found at the calculated location.
    """
    name_parts = name.lstrip('/').split('/')
    for part in name_parts:
        if part == os.path.pardir or os.path.sep in part:
            raise ValueError('Bad path segment: %r' % part)
    filename = os.path.join(os.path.dirname(__file__),
                            'zoneinfo', *name_parts)
    if not os.path.exists(filename):
        # http://bugs.launchpad.net/bugs/383171 - we avoid using this
        # unless absolutely necessary to help when a broken version of
        # pkg_resources is installed.
        try:
            from pkg_resources import resource_stream
        except ImportError:
            resource_stream = None

        if resource_stream is not None:
            return resource_stream(__name__, 'zoneinfo/' + name)
    return open(filename, 'rb')

So, it seems that some pkg_resources version are broken and the zoneinfo directory is missing.

Pendulum needs to use the open_resource from pytz to fix the problem.

[mayfield]

re: version

>>> import pytz
>>> pytz.__version__
'2016.6.1'

[mayfield]

Yeah, how about a patch like this (untested)...

:; git diff pendulum/tz/loader.py | cat
diff --git a/pendulum/tz/loader.py b/pendulum/tz/loader.py
index 296854a..38bebff 100644
--- a/pendulum/tz/loader.py
+++ b/pendulum/tz/loader.py
@@ -15,16 +15,8 @@ class Loader(object):
     @classmethod
     def load(cls, name):
         name = decode(name)
-
-        name_parts = name.lstrip('/').split('/')
-
-        for part in name_parts:
-            if part == os.path.pardir or os.path.sep in part:
-                raise ValueError('Bad path segment: %r' % part)
-
-        filepath = os.path.join(cls.path, *name_parts)
-
-        if not os.path.exists(filepath):
+        try:
+            with pytz.open_resource(name) as f:
+                return Parser._parse(f)
+        except OSError:
             raise ValueError('Unknown timezone [{}]'.format(name))
-
-        return Parser.parse(filepath)

[mayfield]

I tested that and added a couple tests to validate the loader contract (ie. return ValueError when the timezone is not found instead of a FileNotFoundError (OSError for pre py3k). I can submit a PR on that if you like.

[sdispater]

Thanks.

For more simplicity, it's possible to just do the with open_resource() in the public Parser.parse() method.

class Parser(object):

    @classmethod
    def parse(cls, name):
        try:
            with open_resource(name) as fp:
                return cls._parse(fp)
        except OSError:
             raise ValueError('Unknown timezone [{}]'.format(name))

And the Loader to become:

class Loader(object):

    @classmethod
    def load(cls, name):
        name = decode(name)

        return Parser.parse(name)

It would remove a lot of the purpose of the Loader class (except for decoding the name) but I think it would be more logical. 

What do you think?

[mayfield]

Works for me. Is there any reason not to move the decode to Parser too and just deprecate Loader?

[sdispater]

Do you want me to make this change or are you making a pull request?

[mayfield]

I can submit a PR.

[sdispater]

Great! Thank you :-)