tests/stats/test_training_status.py (1)

58-66: ⚠️ Potential issue

Assert that the list is empty and add missing newline.

The test should explicitly verify that no data is returned for the old date. Additionally, the file is missing a newline at the end.

 def test_monthly_training_status_no_data(authed_client: Client):
     end = date(2020, 1, 1)  # Date far in the past with no data
     monthly_training_status = MonthlyTrainingStatus.list(
         end, 1, client=authed_client
     )
     # Should return empty list if no data
-    assert isinstance(monthly_training_status, list)
+    assert monthly_training_status == []
+

src/garth/sso.py (1)

80-86: Verify Python ≥ 3.9 requirement & drop the dict() wrapper

The | union operator only works on Python 3.9+. Double-check that pyproject.toml / setup.cfg already advertises a python_requires >= "3.9" (or bump it) so users on 3.8 don’t get a syntax error.

While you’re here, the extra dict() layer is unnecessary and slightly hurts readability. You can inline a literal:

-    SIGNIN_PARAMS = SSO_EMBED_PARAMS | dict(
-        gauthHost=SSO_EMBED,
-        service=SSO_EMBED,
-        source=SSO_EMBED,
-        redirectAfterAccountLoginUrl=SSO_EMBED,
-        redirectAfterAccountCreationUrl=SSO_EMBED,
-    )
+    SIGNIN_PARAMS = SSO_EMBED_PARAMS | {
+        "gauthHost": SSO_EMBED,
+        "service": SSO_EMBED,
+        "source": SSO_EMBED,
+        "redirectAfterAccountLoginUrl": SSO_EMBED,
+        "redirectAfterAccountCreationUrl": SSO_EMBED,
+    }

🧰 Tools

🪛 Pylint (3.3.7)

[refactor] 80-86: Consider using '{"gauthHost": SSO_EMBED, "service": SSO_EMBED, "source": SSO_EMBED, ... }' instead of a call to 'dict'.

(R1735)

src/garth/__init__.py (1)

31-34: (Nit) keep __all__ alphabetised

Pure style, but alphabetising __all__ simplifies diff noise later.

Not blocking.

Also applies to: 40-40

README.md (1)

621-734: Docs example is great – minor parameter inconsistency

Earlier examples use .list("date", pages) whereas the new training-status snippets use .list(period=1) without a start date. If the API truly differs, maybe add a short note (“DailyTrainingStatus.list() takes only period”) to avoid confusion for readers copy-pasting from other sections.

CLAUDE.md (1)

81-90: Redundant heading sentence

The heading “### Adding New Stats Endpoints” already states the topic; the next line repeats it. Consider deleting the sentence for brevity.

-When adding new stats endpoints (like training status):
+When adding new stats endpoints (like training status):

(or just remove the duplicated phrase entirely)

🧰 Tools

🪛 LanguageTool

[uncategorized] ~83-~83: Possible missing comma found.
Context: ...ng New Stats Endpoints When adding new stats endpoints (like training status): 1. C...

(AI_HYDRA_LEO_MISSING_COMMA)

tests/stats/test_training_status.py (2)

24-33: Consider asserting the expected number of weekly results.

While the test verifies non-empty results, it would be more robust to assert the expected number of entries (e.g., assert len(weekly_training_status) == 4 if the API returns exactly 4 weeks).

---

48-56: Enhance pagination test to verify multi-page retrieval.

The test requests 60 weeks (exceeding the page size of 52) but only checks for non-empty results. Consider adding assertions to verify that pagination actually occurred, such as checking for a minimum number of results or verifying data spans the expected time range.

 def test_weekly_training_status_pagination(authed_client: Client):
     end = date(2025, 6, 11)
     weeks = 60
     weekly_training_status = WeeklyTrainingStatus.list(
         end, weeks, client=authed_client
     )
     assert len(weekly_training_status) > 0
+    # Verify we got data spanning multiple pages
+    # (exact count may vary based on data availability)
+    assert len(weekly_training_status) > 52  # More than one page

src/garth/stats/training_status.py (2)

66-74: Document the single-device limitation more prominently.

The code assumes a single device in multiple places (lines 66 and 82). This limitation should be documented in the class or method docstring to make it clear to users. Consider adding a TODO for future multi-device support.

Would you like me to help implement multi-device support or create an issue to track this enhancement?

Also applies to: 82-93

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 74-74: src/garth/stats/training_status.py#L74
Added line #L74 was not covered by tests

---

154-159: Add missing newline at end of file.

The file is missing a newline character at the end.

 class MonthlyTrainingStatus(TrainingStatus):
     _path: ClassVar[str] = f"{BASE_PATH}/monthly/{{start}}/{{end}}"
     _data_key: ClassVar[str] = "monthlyTrainingStatus"
     _page_size: ClassVar[int] = 12
+

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

src/garth/stats/training_status/daily.py (1)

38-42: _page_size is dead code in the daily implementation

_page_size is never referenced in DailyTrainingStatus.
Because the /latest/{end} endpoint always returns a single day, the
attribute is misleading noise.

-    _page_size: ClassVar[int] = 28

Unless you plan to add real pagination, drop the constant to avoid
confusion.

tests/stats/test_training_status.py (1)

225-233: Unreachable else block – small readability clean-up

The else after a return is redundant. Dropping it shortens the
test helper and silences the linter warning.

-    def mock_connectapi_side_effect(path):
-        # First call returns data, subsequent calls return empty
-        if hasattr(mock_connectapi_side_effect, "call_count"):
-            mock_connectapi_side_effect.call_count += 1
-            return {}
-        else:
-            mock_connectapi_side_effect.call_count = 1
-            return mock_response
+    def mock_connectapi_side_effect(path):
+        # First call returns data, subsequent calls return empty
+        if hasattr(mock_connectapi_side_effect, "call_count"):
+            mock_connectapi_side_effect.call_count += 1
+            return {}
+        mock_connectapi_side_effect.call_count = 1
+        return mock_response

🧰 Tools

🪛 Pylint (3.3.7)

[refactor] 227-232: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it

(R1705)

CLAUDE.md (1)

82-85: Minor punctuation nit

Missing comma after “endpoints”. Helps the long sentence breathe.

-When adding new stats endpoints (like training status):
+When adding new stats endpoints, (like training status):

🧰 Tools

🪛 LanguageTool

[uncategorized] ~83-~83: Possible missing comma found.
Context: ...ng New Stats Endpoints When adding new stats endpoints (like training status): 1. C...

(AI_HYDRA_LEO_MISSING_COMMA)

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

src/garth/stats/_base.py (1)

28-45: ⚠️ Potential issue

Monthly periods are computed in weeks – date ranges are wrong
The period_type fallback treats every non-daily class as weekly.
MonthlyTrainingStatus.list(..., period=6) therefore covers only 5×7 days
instead of ~5 months, and pagination subtracts _page_size weeks rather than
months.

-        period_type = "days" if "daily" in cls._path else "weeks"
+        if "daily" in cls._path:
+            period_type = "days"
+        elif "monthly" in cls._path:
+            period_type = "months"
+        else:       # weekly or other interval
+            period_type = "weeks"

When period_type == "months" you must switch from datetime.timedelta
(which has no months) to dateutil.relativedelta.relativedelta. A minimal,
drop-in patch:

-from datetime import date, timedelta
+from datetime import date, timedelta
+from dateutil.relativedelta import relativedelta
@@
-        start = end - timedelta(**{period_type: period - 1})
+        delta = relativedelta if period_type == "months" else timedelta
+        start = end - delta(**{period_type: period - 1})

and the identical change in the recursive branch (end - delta(...)).
Without this, monthly endpoints silently return inaccurate data.

src/garth/stats/training_status/monthly.py (1)

57-68: Same single-device limitation as weekly parser
See the weekly parser comment – only the first device block is emitted.
Returning all devices’ monthly data would be more faithful to the API.

src/garth/stats/training_status/daily.py (1)

34-38: 🛠️ Refactor suggestion

_path ignores period, causing odd behaviour for period>1
/trainingstatus/latest/{end} can only ever return one day. When callers pass
period > 1, Stats.list repeatedly hits the same latest endpoint with
earlier end dates, which almost certainly yields empty pages and increases
API traffic.

If historical daily data is not available, restrict the public API:

-    def list(cls, end: date | str | None = None, period: int = 1, *, …)
+    def list(cls, end: date | str | None = None, *, …)

or document clearly that only period == 1 is supported.

src/garth/stats/_base.py (1)

63-68: Avoid mutating the source dict while flattening "values"
stat.pop("values") alters the original object that may be reused by
callers/tests. A non-destructive merge is safer:

-        page_dirs = [{**stat, **stat.pop("values")} for stat in page_dirs]
+        page_dirs = [
+            {**{k: v for k, v in stat.items() if k != "values"}, **stat["values"]}
+            for stat in page_dirs
+        ]

This keeps page_dirs independent of the raw API payload.

tests/stats/test_training_status.py (1)

225-233: Minor style nit – else after return is redundant

if hasattr(mock_connectapi_side_effect, "call_count"):
    mock_connectapi_side_effect.call_count += 1
    return {}
else:                       # ← can be deleted
    mock_connectapi_side_effect.call_count = 1
    return mock_response

Removing the else shortens nesting and appeases pylint R1705.

🧰 Tools

🪛 Pylint (3.3.7)

[refactor] 227-232: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it

(R1705)

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

src/garth/stats/training_status/weekly.py (1)

58-69: Still returns only the first device’s data – aggregate across all devices
Behaviour unchanged from the previous review: the early return drops data for users who own multiple devices. Please aggregate entries from all device_data lists before returning.

-        # Get the first device's data (assumes single device for now)
-        for device_data in report_data.values():
-            if isinstance(device_data, list):
-                result = []
-                for entry in device_data:
-                    ...
-                return result
+        result: list[dict] = []
+        for device_data in report_data.values():
+            if isinstance(device_data, list):
+                for entry in device_data:
+                    ...
+                    result.append(entry)
+        return result

src/garth/stats/training_status/monthly.py (1)

35-39: 🛠️ Refactor suggestion

⚠️ Potential issue

Period maths are still based on ISO-weeks → monthly ranges are wrong

Because Stats.list() defaults to period_type = "weeks" for every path that is not daily, calls such as MonthlyTrainingStatus.list(end=date(2025, 6, 1), period=6) will only go back 42 days instead of ~6 calendar months.
The same bug affects the recursion branch that subtracts _page_size.

Override list() here (or, better, fix the heuristic in _base.py) and switch to dateutil.relativedelta(months=…).

+from dateutil.relativedelta import relativedelta
+
     _page_size: ClassVar[int] = 12
+
+    # --- override to use calendar months instead of ISO weeks -------------
+    @classmethod
+    def list(
+        cls,
+        end: date | str | None = None,
+        period: int = 1,
+        *,
+        client: http.Client | None = None,
+    ):
+        client = client or http.client
+        end = format_end_date(end)
+
+        if period > cls._page_size:
+            page = cls.list(end, cls._page_size, client=client)
+            if not page:
+                return []
+            page = (
+                cls.list(
+                    end - relativedelta(months=cls._page_size),
+                    period - cls._page_size,
+                    client=client,
+                )
+                + page
+            )
+            return page
+
+        start = end - relativedelta(months=period - 1)
+        path = cls._path.format(start=start, end=end, period=period)
+        response = client.connectapi(path)
+        stats = [camel_to_snake_dict(s) for s in cls._parse_response(response)]
+        return [cls(**s) for s in stats]

Without this fix every consumer will silently receive truncated data.

src/garth/stats/training_status/daily.py (1)

40-67: period argument still functionally ignored – comment from previous review applies

DailyTrainingStatus relies on an endpoint that always returns a single
day’s record, so Stats.list(..., period=n) never yields n items.
Either implement iterative fetching (one call per date) or drop the
parameter from the public API to stay truthful.

(Tagging as duplicate because this was already raised on an earlier
commit.)

src/garth/sso.py (1)

159-159: Minor nit – optional empty body

data = {} works, but when oauth1.mfa_token is falsy you could pass None to avoid sending an empty form body:

-    data = {"mfa_token": oauth1.mfa_token} if oauth1.mfa_token else {}
+    data = {"mfa_token": oauth1.mfa_token} if oauth1.mfa_token else None

Not critical, just tidies the request.

src/garth/stats/training_status/weekly.py (1)

40-71: Parsing logic duplicated across daily / weekly / monthly – consider a helper
_parse_response in the three training-status classes follows the same nested-dict pattern. An internal utility (e.g. _flatten_training_status(response, *, level_keys) ) would cut duplication and future maintenance cost.

src/garth/stats/training_status/monthly.py (1)

41-71: Parsing logic mirrors the weekly implementation — 👍 but consider defensive copy

The extractor correctly flattens acuteTrainingLoadDTO and copes with unexpected shapes.
Minor: entry.pop("acuteTrainingLoadDTO", {}) mutates the original API dictionary; use entry.get and merge into a new dict to avoid side-effects if the caller reuses the raw response elsewhere.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

src/garth/stats/training_status/weekly.py (1)

35-38: Consider: Explicitly set _period_type = "weeks".

While the base class will correctly infer "weeks" from the path (line 30 in _base.py), explicitly setting _period_type = "weeks" would improve clarity and match the pattern established by DailyTrainingStatus.

📝 Suggested addition for clarity

     _path: ClassVar[str] = (
         "/mobile-gateway/usersummary/trainingstatus/weekly/{start}/{end}"
     )
     _page_size: ClassVar[int] = 52
+    _period_type: ClassVar[str] = "weeks"

tests/stats/test_training_status.py (3)

1-1: Move datetime import to top-level imports.

The datetime import on Line 26 should be moved to the top of the file alongside the date import for consistency and clarity.

♻️ Proposed refactor

-from datetime import date
+from datetime import date, datetime

And remove the inline import:

     if daily_training_status[0].timestamp is not None:
-        from datetime import datetime
-
         assert isinstance(daily_training_status[0].timestamp, datetime)

Also applies to: 26-26

---

215-227: Consider removing the unnecessary patch.

The format_end_date patch appears unnecessary since:

• It returns the same value as the input
• The test focuses on verifying the API path and _period_type
• The mock_client already provides the needed response

♻️ Proposed refactor

     # Test single day request works
-    with patch("garth.stats._base.format_end_date") as mock_format:
-        mock_format.return_value = date(2025, 6, 11)
-
-        result = DailyTrainingStatus.list(
-            date(2025, 6, 11), 1, client=mock_client
-        )
-
-        # Should call the latest endpoint with the end date
-        expected_path = (
-            "/mobile-gateway/usersummary/trainingstatus/latest/2025-06-11"
-        )
-        mock_client.connectapi.assert_called_with(expected_path)
-        assert len(result) == 1
+    result = DailyTrainingStatus.list(
+        date(2025, 6, 11), 1, client=mock_client
+    )
+
+    # Should call the latest endpoint with the end date
+    expected_path = (
+        "/mobile-gateway/usersummary/trainingstatus/latest/2025-06-11"
+    )
+    mock_client.connectapi.assert_called_with(expected_path)
+    assert len(result) == 1

---

274-280: Simplify the mock side_effect implementation.

Two minor improvements:

1. The path parameter is unused (per static analysis). Prefix it with underscore to indicate intentional non-use.
2. The hasattr pattern for tracking call count is convoluted. Consider a simpler approach.

♻️ Proposed refactor

-    def mock_connectapi_side_effect(path):
+    call_count = 0
+    
+    def mock_connectapi_side_effect(_path):
         # First call returns data, subsequent calls return empty
-        if hasattr(mock_connectapi_side_effect, "call_count"):
-            mock_connectapi_side_effect.call_count += 1
+        nonlocal call_count
+        call_count += 1
+        if call_count > 1:
             return {}
-        mock_connectapi_side_effect.call_count = 1
         return mock_response

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Learnt from: CR
Repo: matin/garth PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T00:25:14.763Z
Learning: Applies to stats/**/*.py : Processed statistics and aggregated data should be implemented in the `stats/` module with classes for daily/weekly stats