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.

Sign up for GitHub

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

Replace dateutil.tz with ZoneInfo and tzlocal for better cross-platform compatibility. #40

Merged
merged 10 commits into from
Apr 22, 2024
Merged

Replace dateutil.tz with ZoneInfo and tzlocal for better cross-platform compatibility. #40

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
0dfdd1a
Replace dateutil.tz with ZoneInfo and tzlocal.
junhaoliao Apr 20, 2024
172a9e0
Reformat code; add backports.zoneinfo for Python version < 3.9.
junhaoliao Apr 20, 2024
8d05f3c
Reformat code.
junhaoliao Apr 20, 2024
93c2e8f
Try to fix mypy error.
junhaoliao Apr 20, 2024
f79c492
Update pyproject.toml
junhaoliao Apr 21, 2024
45c0f2b
Update pyproject.toml
junhaoliao Apr 21, 2024
081a5de
Fall back to "UTC" when Exception occurs in getting local timezone name.
junhaoliao Apr 21, 2024
2b79bff
Set tzlocal package version requirement depending on Python version.
junhaoliao Apr 21, 2024
151198f
Replaced abstract tzinfo with ZoneInfo as a concrete type.
junhaoliao Apr 21, 2024
b925550
Merge branch 'main' into tz_win
junhaoliao Apr 21, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion pyproject.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,10 @@ description = "Logging/encoding/decoding using CLP's IR stream format"
readme = "README.md"
requires-python = ">=3.6"
dependencies = [
"backports.zoneinfo >= 0.2.1; python_version<'3.9'",
junhaoliao marked this conversation as resolved.
Show resolved Hide resolved
"clp-ffi-py >= 0.0.9",
"python-dateutil >= 2.7.0",
"typing-extensions >= 3.7.4",
"tzlocal >= 5.2",
junhaoliao marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

btw, according to the document, this lib needs Python >= 3.8. We need to support Python3.7.
PyPI: https://pypi.org/project/tzlocal/

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let's drop it to 5.1 then. That seems to be the last version that supports Python 3.7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

https://pypi.org/project/tzlocal/5.1/

"zstandard >= 0.18.0",
]
classifiers = [
Expand Down
11 changes: 3 additions & 8 deletions src/clp_logging/handlers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
import sys
import time
from abc import ABCMeta, abstractmethod
from datetime import tzinfo
from math import floor
from pathlib import Path
from queue import Empty, Queue
Expand All @@ -13,7 +12,7 @@
from types import FrameType
from typing import Callable, ClassVar, Dict, IO, Optional, Tuple, Union

import dateutil.tz
import tzlocal
from clp_ffi_py.ir import FourByteEncoder
from zstandard import FLUSH_FRAME, ZstdCompressionWriter, ZstdCompressor

Expand Down Expand Up @@ -59,12 +58,8 @@ def _init_timeinfo(fmt: Optional[str], tz: Optional[str]) -> Tuple[str, str]:
if not fmt:
fmt = "yyyy-MM-d H:m:s.A"
if not tz:
tzf: Optional[tzinfo] = dateutil.tz.gettz()
if tzf:
tzp: Path = Path.resolve(Path(tzf._filename)) # type: ignore
tz = "/".join([tzp.parent.name, tzp.name])
else:
tz = "UTC"
tz = tzlocal.get_localzone_name()
Copy link
Member

Choose a reason for hiding this comment

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

from the official doc I didn't see examples w.r.t. error handling. But from the implementation details it seems like it could raise an exception or return None (according to their comments). Shall we add a try block to catch any exceptions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. In fact when I was looking for a Python 3.7 compatible version of the lib, I found this from their docs:

https://pypi.org/project/tzlocal/5.1/#:~:text=tzlocal%204.0%20also%20adds%20an%20official%20function%20get_localzone_name()%20to%20get%20only%20the%20timezone%20name%2C%20instead%20of%20a%20timezone%20object.%20On%20unix%2C%20it%20can%20raise%20an%20error%20if%20you%20don%E2%80%99t%20have%20a%20timezone%20name%20configured%2C%20where%20get_localzone()%20will%20succeed%2C%20so%20only%20use%20that%20if%20you%20need%20the%20timezone%20name.

tzlocal 4.0 also adds an official function get_localzone_name() to get only the timezone name, instead of a timezone object. On unix, it can raise an error if you don’t have a timezone name configured, where get_localzone() will succeed, so only use that if you need the timezone name.


return fmt, tz


Expand Down
10 changes: 8 additions & 2 deletions src/clp_logging/readers.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
from types import TracebackType
from typing import IO, Iterator, List, Match, Optional, Tuple, Type, Union

import dateutil.tz
from clp_ffi_py.ir import FourByteEncoder
from zstandard import ZstdDecompressionReader, ZstdDecompressor

Expand All @@ -31,6 +30,13 @@
VAR_COMPACT_ENCODING,
)

try:
from zoneinfo import ZoneInfo # type: ignore[import-not-found, unused-ignore]
except ImportError:
from backports.zoneinfo import ( # type: ignore[import-not-found, no-redef, unused-ignore]
ZoneInfo,
)


class Log:
"""
Expand Down Expand Up @@ -186,7 +192,7 @@ def read_preamble(self) -> int:
# We do not use the timestamp pattern from the preamble as it may
# be from other languages and therefore incompatible.
# self.timestamp_format = self.metadata[METADATA_TIMESTAMP_PATTERN_KEY]
self.timezone = dateutil.tz.gettz(self.metadata[METADATA_TZ_ID_KEY])
self.timezone = ZoneInfo(self.metadata[METADATA_TZ_ID_KEY])
Copy link
Member

Choose a reason for hiding this comment

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

The class ZoneInfo is an implementation of tzinfo abstracted class, shall we narrow down the type of self.timezone to ZoneInfo instead?
This means we should also update the type annotation in Log._decode

return self.pos

@abstractmethod
Expand Down
Loading