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

Add documentation #48

Merged
merged 4 commits into from
May 2, 2020
Merged

Add documentation #48

merged 4 commits into from
May 2, 2020

Conversation

pganssle
Copy link
Owner

@pganssle pganssle commented Apr 29, 2020

This is a first draft of the module documentation.

Minimum requirements:

Deferred for later expansion:

  • A section on the cache behavior and how it affects the semantics of datetimes
  • Documentation of the behavior during data updates (though that may be subsumed into the section on the cache behavior).
  • Set up some sort of docs-building CI

I've set up RTD to render the documentation here; the main documentation is in the module documentation.

Closes #35.

@pganssle pganssle force-pushed the documentation branch 2 times, most recently from 3d80804 to bb84d81 Compare April 30, 2020 13:32
@pganssle pganssle marked this pull request as ready for review April 30, 2020 14:02
@pganssle pganssle force-pushed the documentation branch 2 times, most recently from 76f5261 to 4ed008a Compare April 30, 2020 14:11
docs/Makefile Outdated Show resolved Hide resolved
docs/conf.py Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved

The following class methods are also available:

.. classmethod:: ZoneInfo.clear_cache(\*, only_keys=None)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This renders as \* which is probably not what you want.

Suggested change
.. classmethod:: ZoneInfo.clear_cache(\*, only_keys=None)
.. classmethod:: ZoneInfo.clear_cache(*, only_keys=None)

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

This is actually a temporary measure to work around this issue in my local editor: marshallward/vim-restructuredtext#14

I planned to remove it before merging, but I would probably forget, so I'm going to leave this unresolved as a reminder 😃

docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
Copy link
Owner Author

@pganssle pganssle left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the reviews @webknjaz and @dereksantibanez!

docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Show resolved Hide resolved
docs/zoneinfo.rst Show resolved Hide resolved

The following class methods are also available:

.. classmethod:: ZoneInfo.clear_cache(\*, only_keys=None)
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch.

This is actually a temporary measure to work around this issue in my local editor: marshallward/vim-restructuredtext#14

I planned to remove it before merging, but I would probably forget, so I'm going to leave this unresolved as a reminder 😃

docs/zoneinfo.rst Outdated Show resolved Hide resolved
tox.ini Outdated Show resolved Hide resolved
@pganssle pganssle force-pushed the documentation branch 3 times, most recently from 1a0923e to bc28c93 Compare April 30, 2020 20:21
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/make.bat Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
docs/zoneinfo.rst Outdated Show resolved Hide resolved
Although it is a somewhat common practice to expose these to end users,
these values are designed to be primary keys for representing the
relevant zones and not necessarily user-facing elements. Projects like
CLDR (the Unicode Common Locale Data Repository) can be used to get
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a link to e.g. http://cldr.unicode.org/translation/timezones ?

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm mildly concerned that there's no https version of that.

I think I'm going to wait until the second pass to add in third-party links. I'd also like to add links to relevant and/or interesting documentation about the IANA zones themselves, but I consider that stuff "nice to have".

Comment on lines +89 to +93
The ``zoneinfo`` module does not directly provide time zone data, and instead
pulls time zone information from the system time zone database or the
first-party PyPI package `tzdata`_, if available. Some systems, including
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For Hypothesis we've used the extlinks extension to create a :pypi: role:

# A dictionary of external sites, mapping aliases to a base URL and a prefix.  
# See http://sphinx-doc.org/ext/extlinks.html and use as e.g. :pypi:`tzdata`
extlinks = {
    "pypi": ("https://pypi.org/project/%s", ""),
    "bpo": ("https://bugs.python.org/issue%s", "bpo-"),
}

Which is a pattern that might also be useful for CPython docs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 we also use this in CherryPy/Cheroot for GitHub links. But I guess it's fine for this specific repo to not be ideal.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I like this a lot. Maybe we should open a bpo for it? I'd have to replace all the :pypi: links manually when moving this into the standard library if I did that here.

docs/zoneinfo.rst Outdated Show resolved Hide resolved
Copy link

@mahmoud mahmoud left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work! Had a couple thoughts/questions, above. Excited to see this in the stdlib!

docs/zoneinfo.rst Show resolved Hide resolved
docs/zoneinfo.rst Show resolved Hide resolved
Comment on lines +140 to +148
.. envvar:: PYTHONTZPATH

This is an :data:`os.pathsep`-separated string containing the time zone
search path to use. It must consist of only absolute rather than relative
paths. Relative components specified in ``PYTHONTZPATH`` will not be used,
but otherwise the behavior when a relative path is specified is
implementation-defined; CPython will raise :exc:`InvalidTZPathWarning`, but
other implementations are free to silently ignore the erroneous component
or raise an exception.
Copy link

@tmontes tmontes May 1, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(following up on @pganssle's tweet, calling for more eyes)

I read this a few times and it is still not 100% clear to me:

  • First it states that "It must consist of only absolute rather than relative paths".
  • Then adds "Relative components (...) will not be used," which I found confusing and in conflict with the previous statement.
  • On top of than it continues with "but otherwise the behavior when a relative path is specified is implementation-defined". Isn't this zoneinfo a single-implementation for CPython's Standard Library? Or does it refer to 3rd party implementations that may interpret PYTHONTZPATH differently?

I have no particular suggestions other than sharing my difficulty in getting a clear picture.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that you're only supposed to put absolute paths in there, and this section is answering the question of "what happens if you put something that is not an absolute path there"? The answer is that you'll either get a warning, an exception or no message, but in no cases will zoneinfo.TZPATH contain a relative path.

Yeah, there's really only one implementation here, but CPython is the reference implementation for the standard library, and other implementations like PyPy may have different behavior. Though practically speaking "implementation-defined" may also mean "we could change this later, since you weren't supposed to be counting on it anyway."

docs/zoneinfo.rst Outdated Show resolved Hide resolved
Comment on lines +356 to +358
A read-only sequence representing the time zone search path -- when
constructing a ``ZoneInfo`` from a key, the key is joined to each entry in
the ``TZPATH``, and the first file found is used.
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(my last 2c)

The complementary statement on constructing ZoneInfo objects felt a bit alien and, to me, is not totally clear. What is meant by the "join" verb? My mind takes me to a SQL-esque join, where the key is matched/looked-up against each TZPATH entry. Is that it?

(parting words: thanks for you work on this!)

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd assumed os.path.join was the inspiration.

Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, it's basically, "We'll treat the key as a relative path in each of the TZPATH entries." I don't know of a better term for this other than "join".

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I now undertand it -- but had to go looking at the contents of the "tzdata dirs". The "join" is in the sense of os.path.join, thus! :-) Thanks.

pganssle and others added 4 commits May 2, 2020 13:59
This could also use a section on the behavior during data updates and a
section on how the cache behavior affects the semantics of datetimes,
but those can be left to later updates.

Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>
Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
@pganssle
Copy link
Owner Author

pganssle commented May 2, 2020

OK, I think this is as good as it needs to be for the moment. Thanks for the reviews everyone!

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.

Add documentation
9 participants