Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

make a tzinfo class at the module level instead of inlining (fixes #81) #188

Closed

Conversation

beckjake
Copy link

@beckjake beckjake commented Aug 6, 2019

Fixes #81 by creating a generic class with a name and an offset, and using instances of that for timezones.

I'm not clear on why snowflake uses type inline to create a fresh class for each timezone, but it breaks picking of results, which is very annoying and means we have to wrap all results with code that does basically this if we want to pickle anything.

This pattern comes straight from the python docs, check out the "FixedOffset" class defined on this page

@drewbanin
Copy link

Is anyone able to take a look at this one?

@smtakeda
Copy link
Contributor

smtakeda commented Aug 9, 2019

no worry, this is in the queue.

@smtakeda
Copy link
Contributor

Writing a test case:

def test_pickle_timestamp_tz(tmpdir, conn_cnx):
    tmp_dir = str(tmpdir.mkdir('pickles'))
    output = os.path.join(tmp_dir, 'tz.pickle')
    expected_tz = None
    with conn_cnx() as con:
        for rec in con.cursor().execute("select '2019-08-11 01:02:03.123 -03:00'::TIMESTAMP_TZ"):
            expected_tz = rec[0]
            with open(output, 'wb') as f:
                pickle.dump(expected_tz, f)
                print(expected_tz)
                print(type(expected_tz))

    with open(output, 'rb') as f:
        read_tz = pickle.load(f)
        print(read_tz)
        assert expected_tz == read_tz

But got this error:

>           read_tz = pickle.load(f)
E           TypeError: __init__() missing 2 required positional arguments: 'name' and 'tzoffset_minutes'

@smtakeda
Copy link
Contributor

This looks like working well.
https:/stub42/pytz/blob/master/src/pytz/__init__.py#L391

pytz.FixedOffset

@smtakeda
Copy link
Contributor

@beckjake and @drewbanin can you review #191 ?

@drewbanin
Copy link

Hey @smtakeda - this looks really great! I didn't know about pytz.FixedOffset. I gave this patch a spin locally and it appears to work really well :)

@smtakeda smtakeda added this to the v1.9.0 milestone Aug 14, 2019
@smtakeda
Copy link
Contributor

working on other branch.

@smtakeda smtakeda closed this Aug 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

TZ objects not pickleable
3 participants