Repository: metricq/metricq-python (language: python)

Issue:
Dealing with naive datetime objects
Passing a naive `datetime` object to `Timestamp.from_datetime` currently leads to:

```
    delta = dt - Timestamp._EPOCH
TypeError: can't subtract offset-naive and offset-aware datetimes
```

Now passing naive objects may be dangerous. And maybe we don't want this. That we need to discuss. I see the following options

- Support both aware and naive `datetime` objects in `from_datetime`. In accordance with `datetime.timestamp()`, assume that naive datetime instances represent local time and use the appropriate mechanisms in `datetime`.
- Don't support this in `from_datetime`, but provide a slightly clearer error.
- We could then also add a `from_naive_datetime` which still allows it, but is more explicit to not make silent mis-assumptions

I tend to the latter variant, but it also bloats up the API somewhat.

I'm currently using

```
Timestamp.from_posix_seconds(local_begin.timestamp())
```

which is less than ideal.

Note: the current documentation is already clear, that aware objects are expected.


Discussion / hints:
I lean towards the third option, but naming it `from_local_datetime`. And of course also adding better error handling.

Fix patch:
diff --git a/metricq/types.py b/metricq/types.py
index 64309e3d..e65bfd53 100644
--- a/metricq/types.py
+++ b/metricq/types.py
@@ -34,6 +34,7 @@
 from functools import total_ordering
 from typing import Any, Dict, Iterator, Union, overload
 
+from dateutil import tz
 from dateutil.parser import isoparse as dateutil_isoparse
 
 from . import history_pb2
@@ -393,19 +394,39 @@ def from_posix_seconds(cls, seconds: float) -> "Timestamp":
 
     @classmethod
     def from_datetime(cls, dt: datetime.datetime) -> "Timestamp":
-        """Create a Timestamp from an aware datetime object
-
-        Args:
-            dt: an aware datetime object
-
-        Returns:
-            :class:`Timestamp`
         """
+        Create a Timestamp from an aware datetime object. If you have a naive
+        datetime object, consider using :meth:`from_local_datetime` instead.
+        """
+        if dt.tzinfo is None:
+            raise TypeError(
+                "cannot to parse naive datetime with `from_datetime`, "
+                "consider using `from_local_datetime` if applicable"
+            )
         delta = dt - Timestamp._EPOCH
         seconds = (delta.days * 24 * 3600) + delta.seconds
         microseconds = seconds * 1000000 + delta.microseconds
         return Timestamp(microseconds * 1000)
 
+    @classmethod
+    def from_local_datetime(cls, dt: datetime.datetime) -> "Timestamp":
+        """
+        Create a timestamp from a naive :class:`datetime` object that uses the
+        local timezone.
+        If you have a proper aware object, use :meth:`from_datetime` instead.
+
+        This function uses `dateutil.tz.gettz()` which uses `/etc/localtime`.
+        Using naive datetime object can be errorprone. Naive local times are
+        ambiguous during daylight savings time adjustments.
+        If you have to use this function, your workflow is probably broken.
+        """
+        if dt.tzinfo is not None:
+            raise TypeError(
+                "refusing to parse aware datetime with `from_local_datetime`, "
+                "use `from_datetime` instead."
+            )
+        return cls.from_datetime(dt.replace(tzinfo=tz.gettz()))
+
     @classmethod
     def from_iso8601(cls, iso_string: str) -> "Timestamp":
         """Create a Timestamp from a `ISO 8601` date-time string.


Test patch:
diff --git a/tests/test_types.py b/tests/test_types.py
index 93f6ae3c..b5a1f806 100644
--- a/tests/test_types.py
+++ b/tests/test_types.py
@@ -1,9 +1,11 @@
+from datetime import datetime
 from logging import getLogger
 from math import isclose
 from random import Random
 from typing import Generator, List, Union
 
 import pytest
+from dateutil import tz
 
 from metricq.exceptions import NonMonotonicTimestamps
 from metricq.types import TimeAggregate, Timedelta, Timestamp, TimeValue
@@ -56,6 +58,30 @@ def time_delta_1d() -> Timedelta:
     return Timedelta(1_000_000_000 * 3600 * 24)
 
 
+datetime_aware_examples = [
+    datetime(year=2021, month=3, day=3, hour=22, tzinfo=tz.tzutc()),
+    datetime(year=2021, month=3, day=3, hour=22, tzinfo=tz.tzoffset("+3", 3)),
+    datetime(year=2021, month=3, day=3, hour=22, tzinfo=tz.tzoffset("-3", -3)),
+]
+
+datetime_naive_examples = [
+    datetime(year=2021, month=3, day=3, hour=22, tzinfo=None),
+    # around DST switches (in Europe...)
+    datetime(year=2023, month=3, day=26, hour=1, minute=23, tzinfo=None),
+    # Well that time actually doesn't exist and that produces... wrong results
+    # datetime(year=2023, month=3, day=26, hour=2, minute=0, tzinfo=None),
+    # datetime(year=2023, month=3, day=26, hour=2, minute=23, tzinfo=None),
+    datetime(year=2023, month=3, day=26, hour=3, minute=0, tzinfo=None),
+    datetime(year=2023, month=3, day=26, hour=3, minute=23, tzinfo=None),
+    # And now for the winter in europe
+    datetime(year=2023, month=10, day=29, hour=1, minute=23, tzinfo=None),
+    datetime(year=2023, month=10, day=29, hour=2, minute=0, tzinfo=None),
+    datetime(year=2023, month=10, day=29, hour=2, minute=23, tzinfo=None),
+    datetime(year=2023, month=10, day=29, hour=3, minute=0, tzinfo=None),
+    datetime(year=2023, month=10, day=29, hour=3, minute=23, tzinfo=None),
+]
+
+
 def test_timedelta_to_string(
     time_delta_random: Timedelta,
     time_delta_1us: Timedelta,
@@ -277,6 +303,34 @@ def test_timeaggregate_from_value_pair_non_monotonic(
         )
 
 
+@pytest.mark.parametrize("datetime_aware", datetime_aware_examples)
+def test_timestamp_from_datetime_aware(datetime_aware: datetime) -> None:
+    assert datetime_aware.tzinfo is not None
+    timestamp = Timestamp.from_datetime(datetime_aware)
+    assert timestamp.posix_ns / 1e9 == pytest.approx(datetime_aware.timestamp(), rel=0)
+
+
+@pytest.mark.parametrize("datetime_naive", datetime_naive_examples)
+def test_timestamp_from_datetime_naive(datetime_naive: datetime) -> None:
+    assert datetime_naive.tzinfo is None
+    with pytest.raises(TypeError):
+        Timestamp.from_datetime(datetime_naive)
+
+
+@pytest.mark.parametrize("datetime_aware", datetime_aware_examples)
+def test_timestamp_from_local_datetime_aware(datetime_aware: datetime) -> None:
+    assert datetime_aware.tzinfo is not None
+    with pytest.raises(TypeError):
+        Timestamp.from_local_datetime(datetime_aware)
+
+
+@pytest.mark.parametrize("datetime_naive", datetime_naive_examples)
+def test_timestamp_from_local_datetime_naive(datetime_naive: datetime) -> None:
+    assert datetime_naive.tzinfo is None
+    timestamp = Timestamp.from_local_datetime(datetime_naive)
+    assert timestamp.posix_ns / 1e9 == pytest.approx(datetime_naive.timestamp(), rel=0)
+
+
 @pytest.mark.parametrize(
     ("date_string", "expected"),
     [