gettz documentation #704

weatherpattern · 2018-04-24T16:27:00Z

Summary of changes

Added parameters, return and examples to gettz.

Closes #647

Pull Request Checklist

Authors have been added to AUTHORS.md
News fragment added in changelog.d. See CONTRIBUTING.md for details

pganssle · 2018-04-24T16:58:53Z

dateutil/tz/tz.py


    class GettzFunc(object):
+        """
+        gettz is designed not to be a public function. It is defined in a closure as a private function and called once. 


This is not true. GettzFunc is not a public function. gettz is very much a public function, otherwise it wouldn't be in __all__ and wouldn't be getting any documentation.

Ah sorry about that. I misunderstood your other comment in the original issue.

pganssle · 2018-04-24T16:59:47Z

You seem to have checked in a .DS_STORE file. You should add that to your global gitignore patterns.

I think this also needs to be rebased against master:

git checkout master
git pull upstream master
git checkout WIP-gettz-documentation
git rebase -i master

If you don't have an upstream remote already, then you need to do:

git remote add upstream git@github.com:dateutil/dateutil.git

weatherpattern · 2018-04-24T19:00:48Z

Thanks for the quick responses. I'll do another pass as soon as I'm able, but it might take a couple of days.

add name added PR dateutil#704

weatherpattern · 2018-04-25T01:56:03Z

I rebased my branch, which took a while. I correct the gettz description and added my name in the correct position. Thanks for your help with on this, if you'd like me to fix anything else, please let me know.

pganssle · 2018-04-25T02:06:29Z

Something seems to have gone wrong with the rebase because this PR now includes a bunch of commits unrelated to it. I'll go in there and clean up the history.

weatherpattern · 2018-04-25T02:14:34Z

Yeah, sorry about that, it was a bit of a mess trying to get all the merge conflicts sorted out. Thanks in advance for cleaning up the history.

pganssle · 2018-04-25T02:52:30Z

OK, I've significantly cleaned up the history, including squashing all the fixup stuff. You'll need to do git pull --force before making any additional changes or pushing to your branch.

Also, I noticed that your user.email is set to raymondcha@MacBook-Air-2.local. If that is not the e-mail you use on Github (and I assume it is not), Github won't know to associate those commits with you.

If you don't want your e-mail address in the git history, you can set it to weatherpattern@users.noreply.github.com and your commits will be associated with your Github username, if you care about that.

I can rewrite the git history to use that e-mail address if you'd prefer.

pganssle · 2018-04-25T02:55:13Z

dateutil/tz/tz.py

+        ..code-block::
+
+            >>> from dateutil.tz import tzutc, UTC
+            >>> tgettz('AEST-10AEDT-11,M10.1.0/2,M4.1.0/3')


The UTC and tzutc imports are unnecessary, and tgettz is definitely not right.

weatherpattern · 2018-04-25T12:12:46Z

Hi for the clean up. I did a git pull --force and my commit matches upstream, which is a good sign.
Also I changed my global email address, but when you have some time, if you could rewrite the git history to use that no-reply email that would be great. Thanks.

pganssle · 2018-04-25T13:29:28Z

OK done. Note that you'll need to do another git pull --force, since I rewrote the history again.

weatherpattern · 2018-04-26T00:21:03Z

Hello! I removed the import lines and corrected the gettz() typo. Had trouble with the pull --force again but I think I got it sorted out. Thanks for your patience!

pganssle · 2018-04-26T01:08:10Z

dateutil/tz/tz.py

+        Given a variety of inputs, like time zone name and ``TZ`` varible,
+        return a time zone.

        :param object:


This is not actually the interface. GettzFunc is a class representing a callable function. object is the name of the base type of all objects in Python 3 ("new style classes"), and so GettzFunc is a subclass of object for backwards compatibility with Python 2.

I realize it's confusing, but what's being written here is actually the documentation for tz.gettz, which is an instance of GettzFunc, but for practical purposes is equivalent to a function.

When you call tz.gettz(), it actually invokes GettzFunc.__call__, which itself is a thin wrapper around GettzFunc.nocache.

The reason it's built this way is partially for performance reasons, but mostly because I want tz.gettz to satisfy the following invariant:

assert tz.gettz(name) is tz.gettz(name)

So I need to store a reference to the arguments that have been passed to tz.gettz and all the results, so that I can return the same object if I get the same arguments. Those get stored on the GettzFunc instance in the _cache. Because you might want the old semantics, I've exposed a "nocache" variant that bypasses the cache: tz.gettz.nocache(name), and since cache misses are identical to calls to nocache, __call__ is implemented in terms of nocache.

There are also functions on gettz for manipulating the cache, like clear_cache.

Thanks for the extended comments. I'm going to a little more time digest and a bit more research and make another pass. More coming.

Do you have an example (could be anywhere) of how much detail you prefer in the docstring?

OK I took another pass and it's hopefully more accurate now. I also added the assertion as an example.

A couple questions:
Your explanation above was really helpful. Do you want me to make a new issue to put that information into the individual methods and functions as comments?

Also, I wanted to double check:

>>tz.gettz() is tz.gettz() False

This is as designed (I think.) Because if name=None, then the time zone is not cached. Is that correct?

Do you have an example (could be anywhere) of how much detail you prefer in the docstring?

I'm not really satisfied with the dateutil documentation in general. I personally originated the functions (and hence the docstrings) for isoparse and default_tzinfo, so something like that is likely what I would write, but I actually need to do a full re-organization of the documentation, and I'm not sure where information should go for each function / class.

I'm thinking in the end that however it's organized, somewhere in the documentation, the functions and classes would be documented roughly with a man page level of detail.

Your explanation above was really helpful. Do you want me to make a new issue to put that information into the individual methods and functions as comments?

Sure.

Also, I wanted to double check:

>>> tz.gettz() is tz.gettz False

This is as designed (I think.) Because if name=None, then the time zone is not cached. Is that correct?

Yes and no. Ideally that would be True even for None, but since tz.gettz() represents local time it's made a bit more complicated by the fact that local time might change over the runtime of a program (which leads to a bunch of bugs, but we don't want to add to the bugginess by adding to the problem). As such, I'd rather err on the side of the previous non-cached behavior until we can work out how best to figure out if the "local zone" cache has been invalidated by changes in the execution environment.

pganssle

I think at this point the most productive thing would be for me to make an accurate description with the right level of detail. This is a complicated function and it's a bit much to ask anyone else to understand precisely what it does and what it is intended to do.

pganssle · 2018-04-30T18:05:14Z

dateutil/tz/tz.py

+            A time zone name, a ``TZ`` variable, or ``None``.
+
+        :return:
+            A :class:`dateutil.tz.tzfile`. If name is ``None``,


This is also not accurate. If you look at the code, it can return tzfile, tzlocal, tzstr, possibly others.

This function is that it takes a string representing a time zone and returns the best-matching tzinfo subclass instance. The specific subclass returned is not part of the function's contract.

pganssle · 2018-04-30T18:05:29Z

dateutil/tz/tz.py

        tzlocal_classes += (tzwinlocal,)

    class GettzFunc(object):
+        """gettz(name=None)


The function prototype will be auto-generated by sphinx and should not be included.

Maybe there is another way, but without this added line, generating the docs locally, gettz() is returned without the name=None.

weatherpattern · 2018-04-30T18:50:35Z

I agree that it would be good pause and regroup on this. If I can be of help, let me know. Thanks your time and all your responses.

Add changelog for PR dateutil#704

pganssle · 2018-05-08T15:39:54Z

@weatherpattern Thanks for your hard work on this!

weatherpattern pushed a commit to weatherpattern/dateutil that referenced this pull request Apr 24, 2018

added PR dateutil#704

47d1a8f

pganssle reviewed Apr 24, 2018

View reviewed changes

pganssle added the documentation label Apr 24, 2018

pganssle added this to the 2.7.3 milestone Apr 24, 2018

weatherpattern pushed a commit to weatherpattern/dateutil that referenced this pull request Apr 25, 2018

resolved conflict used all names

9f0162a

add name added PR dateutil#704

pganssle force-pushed the WIP-gettz-documentation branch from f1140ed to 075759d Compare April 25, 2018 02:49

pganssle pushed a commit to weatherpattern/dateutil that referenced this pull request Apr 25, 2018

Add changelog for PR dateutil#704

140af48

pganssle reviewed Apr 25, 2018

View reviewed changes

pganssle force-pushed the WIP-gettz-documentation branch from 075759d to 7e63884 Compare April 25, 2018 13:29

pganssle pushed a commit to weatherpattern/dateutil that referenced this pull request Apr 25, 2018

Add changelog for PR dateutil#704

f6a54c2

pganssle reviewed Apr 26, 2018

View reviewed changes

pganssle reviewed Apr 30, 2018

View reviewed changes

weatherpattern mentioned this pull request May 1, 2018

Add comments to GettzFunc #708

Open

pganssle force-pushed the WIP-gettz-documentation branch from d7903ff to df4747b Compare May 8, 2018 14:55

pganssle pushed a commit to weatherpattern/dateutil that referenced this pull request May 8, 2018

Add changelog for PR dateutil#704

0ab0e5c

Add documentation for tz.gettz

b54b8b1

Add changelog for PR dateutil#704

pganssle force-pushed the WIP-gettz-documentation branch from df4747b to c7fb96e Compare May 8, 2018 15:18

weatherpattern and others added 3 commits May 8, 2018 11:19

Add Raymond Cha to authors

6a46f01

Fix gettz documenter type

c078870

Improve documentation for gettz

b403a00

pganssle force-pushed the WIP-gettz-documentation branch from c7fb96e to b403a00 Compare May 8, 2018 15:19

pganssle merged commit 4bc9095 into dateutil:master May 8, 2018

pganssle mentioned this pull request May 9, 2018

Release 2.7.3 #717

Merged

gettz documentation #704

gettz documentation #704

Uh oh!

Conversation

weatherpattern commented Apr 24, 2018 • edited by pganssle Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary of changes

Pull Request Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pganssle commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weatherpattern commented Apr 24, 2018

Uh oh!

weatherpattern commented Apr 25, 2018

Uh oh!

pganssle commented Apr 25, 2018

Uh oh!

weatherpattern commented Apr 25, 2018

Uh oh!

pganssle commented Apr 25, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weatherpattern commented Apr 25, 2018

Uh oh!

pganssle commented Apr 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

weatherpattern commented Apr 26, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weatherpattern Apr 28, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weatherpattern Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pganssle left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

weatherpattern commented Apr 30, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pganssle commented May 8, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

weatherpattern commented Apr 24, 2018 •

edited by pganssle

Loading

pganssle commented Apr 24, 2018 •

edited

Loading

pganssle commented Apr 25, 2018 •

edited

Loading

weatherpattern Apr 28, 2018 •

edited

Loading

weatherpattern Apr 30, 2018 •

edited

Loading

weatherpattern commented Apr 30, 2018 •

edited

Loading