Fix #133: Allow deepcopy on Timezone classes.

[AndreasBackx]

I've added default values to the Timezone classes so that they can be deepcopied. I think it's appropriate to provide these as datetime.tzinfo also has an empty constructor.

[sethmlarson]

It's dangerous to use mutable default parameters. I'd suggest using None values and converting to the proper defaults within __init__.

[sethmlarson]

Might want to add flake8 to be a part of Travis CI to catch things like this @sdispater

[AndreasBackx]

Good to know, I can change that in a follow-up commit. Let me know what else needs to be changed before it can be merged so I can make a new commit and squash.

[sethmlarson]

Beyond this I think it'd be a good idea to implement __copy__() and __deepcopy__() which are referenced in the Python docs for the copy module rather than adding default parameters. TBH I've never used these methods myself so some experimentation might be required.

[pganssle]

Why would you want to be able to deepcopy a TimeZone object? Not saying I haven't done the same sort of thing before, but my understanding is that tzinfo subclasses are intended to be singletons or singleton-like.

[sethmlarson]

@pganssle If you're deep-copying something that holds onto an object that holds onto an object ... etc ... that eventually holds onto tzinfo you want copy.deepcopy to be able to give you a non-error back.

[pganssle]

@SethMichaelLarson Hm.. But is this desirable if tzinfo objects are not in fact implemented as singletons but you're hoping for them to be?

One reason this can be a problem is that the logic for inter-zone comparison and intra-zone comparison is slightly different, and two things are considered different if tz1 is not tz2, even if tz1 == tz2.

[sethmlarson]

It's not desirable for them to be copied if they're meant to be singletons. People who know this library's internals should answer your question better than me. I'm just saying that this object should probably not raise an exception when it's copied otherwise it makes all objects that want to be copied and also hold onto this object into more difficult beasts.

[sdispater]

@pganssle I agree that, ideally, tzinfo subclasses should be singletons. However, pendulum took an approach similar to pytz regarding timezones handling in its first stable version. This is something that will likely change in the next major version.

[AndreasBackx]

@sdispater so what should be the eventual solution? Default None values, the implementation of __copy__ and __deepcopy__, or both?

[sdispater]

@AndreasBackx The TimezoneInfo class is not meant to be used directly. So I think the best approach here would be to implement __copy__ and __deepcopy__.

[AndreasBackx]

@sdispater I don't think we need to implement __copy__ as that seems to work fine. Should I perhaps add tests as well just to be sure?

I'd see these methods calling the constructors again with the same parameters or do you suggest another way?

[sethmlarson]

@AndreasBackx You don't need to implement __copy__ because you're never directly copying a TimezoneInfo object. You might as well implement it for completeness sake.

[AndreasBackx]

I found something while looking at the Django's source for their tzinfo subclasses:

Special requirement for pickling: A tzinfo subclass must have an __init__() method that can be called with no arguments, else it can be pickled but possibly not unpickled again. This is a technical requirement that may be relaxed in the future.

Snippet is from the Python docs.

So we should follow this requirement and provide either default None or primitive type values or add __getinitargs__(self) that provides these. However I am not sure whether that method is used by deepcopy.

I would go with the default values and not overcomplicate things with __getinitargs__(self).

[sdispater]

@AndreasBackx This is for pickling which has different requirements.

So we can go with only implementing __deepcopy__ I think.

[AndreasBackx]

@sdispater alright. I'm currently on holiday and get back at the beginning of August so I won't be able to make a new commit until then. If I haven't committed anything by then (let's say the first week of August), don't be afraid to ping me.

[bolkedebruin]

@AndreasBackx friendly nudge. We could really use this for Apache Airflow which is dependent on the possibility of pickling

[sdispater]

Fixed by #161

[AndreasBackx]

@bolkedebruin sorry for leaving this in the dust. I've been getting swamped these last few weeks. Thanks for putting forth your own PR. 👍