From 4a915a274c30bae0d9f618e386e4746372052461 Mon Sep 17 00:00:00 2001
From: Thomas Ilsche <thomas.ilsche@tu-dresden.de>
Date: Thu, 13 Apr 2023 10:37:45 +0200
Subject: [PATCH] feat: deprecate dict methods

towards #142
---
 docs/upgrading.rst                   | 11 +++++++-
 metricq/timeseries/time_aggregate.py | 38 ++++++++++++++++++++++++++--
 metricq/timeseries/time_value.py     | 14 ++++++++--
 setup.cfg                            |  1 +
 setup.py                             |  1 +
 5 files changed, 60 insertions(+), 5 deletions(-)

diff --git a/docs/upgrading.rst b/docs/upgrading.rst
index 0d32afa8..f47b3117 100644
--- a/docs/upgrading.rst
+++ b/docs/upgrading.rst
@@ -147,4 +147,13 @@ If you have imported types from it directly you should change to import directly
 * :class:`TimeValue`
 * :class:`TimeAggregate`
 * :class:`Metric` (available in `metricq` since `5.0`)
-* :class:`JsonDict` (available in `metricq` since `5.0`)
\ No newline at end of file
+* :class:`JsonDict` (available in `metricq` since `5.0`)
+
+
+Deprecation of `dict` methods
+^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
+
+The methods :meth:`TimeAggregate.dict` and :meth:`TimeValue.dict` have been deprecated.
+Use the individual fields instead.
+The using code has more context and should know better which fields to include.
+In particular, whether to use :attr:`TimeAggregate.mean_sum` or :attr:`TimeAggregate.mean_integral` and which :attr:`TimeValue.timestamp` type to use.
diff --git a/metricq/timeseries/time_aggregate.py b/metricq/timeseries/time_aggregate.py
index 904fff94..9d7605c7 100644
--- a/metricq/timeseries/time_aggregate.py
+++ b/metricq/timeseries/time_aggregate.py
@@ -1,5 +1,7 @@
 from dataclasses import dataclass
-from typing import Any
+from typing import Any, Dict
+
+from deprecated.sphinx import deprecated
 
 from .. import history_pb2
 from ..exceptions import NonMonotonicTimestamps
@@ -97,6 +99,14 @@ def integral_s(self) -> float:
 
     @property
     def mean(self) -> float:
+        """
+        Mean value of this aggregate.
+        This is the general way to access the mean value if nothing specific
+        is known about the underlying raw data.
+
+        It may involve a heuristic to decide between :attr:`mean_integral` and
+        :attr:`mean_sum`. Currently, it defaults to :attr:`mean_integral`.
+        """
         if self.active_time.ns > 0:
             return self.mean_integral
         else:
@@ -104,13 +114,37 @@ def mean(self) -> float:
 
     @property
     def mean_integral(self) -> float:
+        """
+        Mean value of this aggregate, calculated from the integral.
+        Use this if you want to explicitly force this property.
+
+        This should only be `NaN` if the aggregate interval is outside the
+        measured time.
+        """
         return self.integral_ns / self.active_time.ns
 
     @property
     def mean_sum(self) -> float:
+        """
+        Mean value of this aggregate, calculated from the sum.
+        This can be useful when the underlying metric should be at regular
+        intervals, but there are gaps in the data. Otherwise,
+        :attr:`mean_integral` or just :attr:`mean` are more appropriate.
+
+        This value will be `NaN` if there are no raw data points in the
+        aggregate interval.
+        """
         return self.sum / self.count
 
-    def dict(self) -> dict[str, Any]:
+    @deprecated(
+        version="5.0.0",
+        reason=(
+            "Use the individual properties instead and chose between "
+            "`mean_integral` and `mean_sum` based on your use-case"
+        ),
+    )
+    # using Dict as return type to work around https://github.com/python/mypy/issues/15047
+    def dict(self) -> Dict[str, Any]:
         """
         returns a dict representing the TimeAggregate instance.
         Keys are `timestamp`, `minimum`, `mean`, `maximum`, and `count`.
diff --git a/metricq/timeseries/time_value.py b/metricq/timeseries/time_value.py
index 6384d2bc..93d67371 100644
--- a/metricq/timeseries/time_value.py
+++ b/metricq/timeseries/time_value.py
@@ -1,6 +1,8 @@
 from collections.abc import Iterator
 from dataclasses import dataclass
-from typing import Any
+from typing import Any, Dict
+
+from deprecated.sphinx import deprecated
 
 from .timestamp import Timestamp
 
@@ -24,7 +26,15 @@ class TimeValue:
     def __iter__(self) -> Iterator[Timestamp | float]:
         return iter([self.timestamp, self.value])
 
-    def dict(self) -> dict[str, Any]:
+    @deprecated(
+        version="5.0.0",
+        reason=(
+            "Use the individual properties instead and select an appropriate "
+            "timestamp type."
+        ),
+    )
+    # using Dict as return type to work around https://github.com/python/mypy/issues/15047
+    def dict(self) -> Dict[str, Any]:
         """
         returns a dict representing the TimeValue instance.
         Keys are `timestamp` and `value`
diff --git a/setup.cfg b/setup.cfg
index 44d68f33..a3cfdb05 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -49,6 +49,7 @@ test =
 typing =
     mypy>=1.1.0
     mypy-protobuf
+    types-Deprecated
     types-setuptools
     types-protobuf
     types-python-dateutil
diff --git a/setup.py b/setup.py
index da87e6ed..f435267d 100755
--- a/setup.py
+++ b/setup.py
@@ -271,6 +271,7 @@ def run(self):
         #  be available with the next aio-pika release
         "aio-pika~=6.7, >=6.7.1",
         get_protobuf_requirement(),
+        "Deprecated~=1.2.13",
         "python-dateutil ~= 2.8, >=2.8.1",
         "yarl",
         "setuptools",