fix: address PR review comments

- Add full Google-style docstring to _normalize_datetime (#10)
- Adopt ColumnHelper in test_acv.py for consistency (#7)
- Rename misleading test_within_group_with_user_column_named_total_stores
  to test_within_group_ignores_unrelated_extra_columns (#4)
- Add group_col/within_group examples to docs/metrics.md (#9)
- Fix PR description: product_col is str | None, not list (#2)

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Author: mvanwyk

docs/metrics.md

@@ -65,3 +65,21 @@ print(pct.df)
 # 1         502       2        50.0
 # 2         503       1        25.0
 ```
+
+Use `group_col` to add extra grouping dimensions, and `within_group=True` to compute the
+percentage relative to stores within each group rather than all stores:
+
+```python
+df = pd.DataFrame({
+    "store_id": [10, 20, 30, 40, 10],
+    "product_id": [501, 501, 502, 502, 502],
+    "region": ["North", "North", "South", "South", "North"],
+    "unit_spend": [5.99, 3.49, 4.00, 6.00, 2.50],
+})
+
+# Percentage relative to all stores (default)
+pct = PctOfStores(df, group_col="region")
+
+# Percentage relative to stores within each region
+pct_within = PctOfStores(df, group_col="region", within_group=True)
+```

pyretailscience/utils/date.py

@@ -9,7 +9,21 @@
 
 
 def _normalize_datetime(date_val: datetime | str) -> datetime:
-    """Convert string or datetime to timezone-aware datetime object."""
+    """Convert a string or datetime to a timezone-aware datetime object.
+
+    Strings are parsed as ``%Y-%m-%d`` and localized to UTC.  Naive datetimes
+    are made aware by attaching UTC.  Already-aware datetimes are returned
+    unchanged.
+
+    Args:
+        date_val (datetime | str): A date string (``YYYY-MM-DD``) or datetime to normalize.
+
+    Returns:
+        datetime: A timezone-aware datetime in UTC.
+
+    Raises:
+        TypeError: If *date_val* is neither a ``str`` nor a ``datetime``.
+    """
     if isinstance(date_val, str):
         # Convert string to timezone-aware datetime
         return datetime.strptime(date_val, "%Y-%m-%d").replace(tzinfo=timezone.utc)

tests/metrics/distribution/test_acv.py

@@ -7,6 +7,9 @@
 from pandas.testing import assert_frame_equal
 
 from pyretailscience.metrics.distribution.acv import Acv
+from pyretailscience.options import ColumnHelper
+
+cols = ColumnHelper()
 
 
 class TestAcv:
@@ -16,10 +19,10 @@ def test_acv_total_no_grouping(self):
         """Test total ACV across all transactions without grouping."""
         df = pd.DataFrame(
             {
-                "customer_id": [1, 2, 3, 1, 2],
-                "store_id": [101, 101, 102, 102, 103],
-                "product_id": [10, 20, 30, 40, 50],
-                "unit_spend": [500_000.0, 750_000.0, 300_000.0, 600_000.0, 350_000.0],
+                cols.customer_id: [1, 2, 3, 1, 2],
+                cols.store_id: [101, 101, 102, 102, 103],
+                cols.product_id: [10, 20, 30, 40, 50],
+                cols.unit_spend: [500_000.0, 750_000.0, 300_000.0, 600_000.0, 350_000.0],
             }
         )
         result = Acv(df).df
@@ -31,15 +34,15 @@ def test_acv_grouped_by_store(self, input_type):
         """Test ACV grouped by store returns correct per-store values for both input types."""
         pdf = pd.DataFrame(
             {
-                "store_id": [101, 101, 102, 102, 103],
-                "unit_spend": [400_000.0, 600_000.0, 300_000.0, 200_000.0, 500_000.0],
+                cols.store_id: [101, 101, 102, 102, 103],
+                cols.unit_spend: [400_000.0, 600_000.0, 300_000.0, 200_000.0, 500_000.0],
             }
         )
         df = ibis.memtable(pdf) if input_type == "ibis" else pdf
-        result = Acv(df, group_col="store_id").df.sort_values("store_id").reset_index(drop=True)
+        result = Acv(df, group_col=cols.store_id).df.sort_values(cols.store_id).reset_index(drop=True)
         expected = pd.DataFrame(
             {
-                "store_id": [101, 102, 103],
+                cols.store_id: [101, 102, 103],
                 "acv": [1.0, 0.5, 0.5],
             }
         )
@@ -49,15 +52,15 @@ def test_acv_group_col_list(self):
         """Test ACV grouped by multiple columns."""
         df = pd.DataFrame(
             {
-                "store_id": [101, 101, 102],
+                cols.store_id: [101, 101, 102],
                 "region": ["North", "North", "South"],
-                "unit_spend": [1_000_000.0, 500_000.0, 2_000_000.0],
+                cols.unit_spend: [1_000_000.0, 500_000.0, 2_000_000.0],
             }
         )
-        result = Acv(df, group_col=["store_id", "region"]).df.sort_values("store_id").reset_index(drop=True)
+        result = Acv(df, group_col=[cols.store_id, "region"]).df.sort_values(cols.store_id).reset_index(drop=True)
         expected = pd.DataFrame(
             {
-                "store_id": [101, 102],
+                cols.store_id: [101, 102],
                 "region": ["North", "South"],
                 "acv": [1.5, 2.0],
             }
@@ -68,37 +71,37 @@ def test_acv_with_nan_values(self):
         """Test that NaN values are excluded from the ACV sum."""
         df = pd.DataFrame(
             {
-                "store_id": [101, 101, 102],
-                "unit_spend": [1_000_000.0, np.nan, 500_000.0],
+                cols.store_id: [101, 101, 102],
+                cols.unit_spend: [1_000_000.0, np.nan, 500_000.0],
             }
         )
-        result = Acv(df, group_col="store_id").df.sort_values("store_id").reset_index(drop=True)
+        result = Acv(df, group_col=cols.store_id).df.sort_values(cols.store_id).reset_index(drop=True)
         expected = pd.DataFrame(
             {
-                "store_id": [101, 102],
+                cols.store_id: [101, 102],
                 "acv": [1.0, 0.5],
             }
         )
         assert_frame_equal(result, expected)
 
     def test_acv_missing_column_raises(self):
         """Test that missing unit_spend column raises ValueError."""
-        df = pd.DataFrame({"customer_id": [1, 2], "store_id": [101, 102]})
+        df = pd.DataFrame({cols.customer_id: [1, 2], cols.store_id: [101, 102]})
         with pytest.raises(ValueError, match="missing"):
             Acv(df)
 
-    def test_acv_missing_group_col_column_raises(self):
+    def test_acv_missing_group_col_raises(self):
         """Test that missing group_col column raises ValueError."""
-        df = pd.DataFrame({"unit_spend": [100.0, 200.0]})
+        df = pd.DataFrame({cols.unit_spend: [100.0, 200.0]})
         with pytest.raises(ValueError, match="missing"):
-            Acv(df, group_col="store_id")
+            Acv(df, group_col=cols.store_id)
 
     def test_acv_custom_scale_factor(self):
         """Test ACV with a custom scale factor."""
         df = pd.DataFrame(
             {
-                "store_id": [101, 102],
-                "unit_spend": [5_000.0, 10_000.0],
+                cols.store_id: [101, 102],
+                cols.unit_spend: [5_000.0, 10_000.0],
             }
         )
         result = Acv(df, acv_scale_factor=1_000).df
@@ -108,11 +111,11 @@ def test_acv_custom_scale_factor(self):
     @pytest.mark.parametrize("scale_factor", [0, -1_000])
     def test_acv_non_positive_scale_factor_raises(self, scale_factor):
         """Test that zero or negative acv_scale_factor raises ValueError."""
-        df = pd.DataFrame({"unit_spend": [500_000.0, 1_000_000.0]})
+        df = pd.DataFrame({cols.unit_spend: [500_000.0, 1_000_000.0]})
         with pytest.raises(ValueError, match="acv_scale_factor must be positive"):
             Acv(df, acv_scale_factor=scale_factor)
 
     def test_acv_invalid_type_raises(self):
         """Test that passing a non-DataFrame/Table raises TypeError."""
         with pytest.raises(TypeError, match="pandas DataFrame or an Ibis Table"):
-            Acv({"unit_spend": [100.0]})
+            Acv({cols.unit_spend: [100.0]})

tests/metrics/distribution/test_pct_of_stores.py

@@ -220,14 +220,14 @@ def test_within_group_ignored_without_group_col(self):
         result_without = PctOfStores(df, within_group=False).df.sort_values(cols.product_id).reset_index(drop=True)
         assert_frame_equal(result_with, result_without)
 
-    def test_within_group_with_user_column_named_total_stores(self):
-        """Test that a user column named _total_stores does not collide with internal temp column."""
+    def test_within_group_ignores_unrelated_extra_columns(self):
+        """Test that extra columns in the input don't affect within_group computation."""
         df = pd.DataFrame(
             {
                 cols.store_id: [10, 20, 30, 40, 10],
                 cols.product_id: [501, 501, 502, 502, 502],
                 "region": ["North", "North", "South", "South", "North"],
-                "_total_stores": [100, 200, 300, 400, 100],
+                "some_metric": [100, 200, 300, 400, 100],
                 cols.unit_spend: [5.99, 3.49, 4.00, 6.00, 2.50],
             }
         )