Add training status support #130
Conversation
- Add DailyTrainingStatus, WeeklyTrainingStatus, MonthlyTrainingStatus classes - Custom parsing logic for complex nested API responses - Comprehensive test coverage with VCR cassettes - Documentation in README with usage examples - Follows existing patterns for consistency and DRY principles Closes #129 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
""" WalkthroughThis change introduces support for retrieving daily, weekly, and monthly training status data from the Garmin Connect API. It adds new dataclasses to model each interval, implements data extraction and pagination logic, updates public exports, extends documentation with usage examples, and provides comprehensive tests for the new functionality and edge cases. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StatsClass as Daily/Weekly/MonthlyTrainingStatus
participant Client as HTTP Client
participant API as Garmin Connect API
User->>StatsClass: .list(end, period, client)
StatsClass->>Client: GET /mobile-gateway/usersummary/trainingstatus/{interval}/...
Client->>API: HTTP request
API-->>Client: JSON response
Client-->>StatsClass: Response data
StatsClass->>StatsClass: Extract and flatten data
StatsClass->>User: List of TrainingStatus objects
Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
📜 Recent review detailsConfiguration used: .coderabbit.yaml ⛔ Files ignored due to path filters (5)
📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
🧰 Additional context used🪛 Pylint (3.3.7)src/garth/stats/training_status/daily.py[refactor] 10-10: Too many instance attributes (23/7) (R0902) ✨ Finishing Touches
🧪 Generate Unit Tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #130 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 43 48 +5
Lines 1793 2083 +290
==========================================
+ Hits 1793 2083 +290
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
tests/stats/test_training_status.py (1)
58-66:⚠️ Potential issueAssert 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 == [] +
🧹 Nitpick comments (8)
src/garth/sso.py (1)
80-86: Verify Python ≥ 3.9 requirement & drop thedict()wrapperThe
|union operator only works on Python 3.9+. Double-check thatpyproject.toml/setup.cfgalready advertises apython_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__alphabetisedPure 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 inconsistencyEarlier 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 onlyperiod”) to avoid confusion for readers copy-pasting from other sections.CLAUDE.md (1)
81-90: Redundant heading sentenceThe 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) == 4if 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 pagesrc/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 +
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
tests/stats/cassettes/test_daily_training_status.yamlis excluded by!tests/**/cassettes/**tests/stats/cassettes/test_monthly_training_status.yamlis excluded by!tests/**/cassettes/**tests/stats/cassettes/test_monthly_training_status_no_data.yamlis excluded by!tests/**/cassettes/**tests/stats/cassettes/test_weekly_training_status.yamlis excluded by!tests/**/cassettes/**tests/stats/cassettes/test_weekly_training_status_pagination.yamlis excluded by!tests/**/cassettes/**
📒 Files selected for processing (7)
CLAUDE.md(1 hunks)README.md(1 hunks)src/garth/__init__.py(2 hunks)src/garth/sso.py(1 hunks)src/garth/stats/__init__.py(2 hunks)src/garth/stats/training_status.py(1 hunks)tests/stats/test_training_status.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`tests/**`: - test functions shouldn't have a return type hint - it's ok to use `assert` instead of `pytest.assume()`
tests/**: - test functions shouldn't have a return type hint
- it's ok to use
assertinstead ofpytest.assume()
tests/stats/test_training_status.py
🧬 Code Graph Analysis (3)
src/garth/stats/__init__.py (1)
src/garth/stats/training_status.py (3)
DailyTrainingStatus(141-144)MonthlyTrainingStatus(155-158)WeeklyTrainingStatus(148-151)
src/garth/__init__.py (1)
src/garth/stats/training_status.py (3)
DailyTrainingStatus(141-144)MonthlyTrainingStatus(155-158)WeeklyTrainingStatus(148-151)
tests/stats/test_training_status.py (3)
src/garth/stats/training_status.py (3)
DailyTrainingStatus(141-144)MonthlyTrainingStatus(155-158)WeeklyTrainingStatus(148-151)src/garth/http.py (1)
Client(19-244)tests/conftest.py (3)
vcr(79-82)authed_client(65-75)client(21-22)
🪛 Pylint (3.3.7)
src/garth/sso.py
[refactor] 80-86: Consider using '{"gauthHost": SSO_EMBED, "service": SSO_EMBED, "source": SSO_EMBED, ... }' instead of a call to 'dict'.
(R1735)
src/garth/stats/training_status.py
[refactor] 15-15: Too many instance attributes (24/7)
(R0902)
[refactor] 46-46: Too many return statements (9/6)
(R0911)
[refactor] 46-46: Too many branches (14/12)
(R0912)
🪛 GitHub Check: codecov/patch
src/garth/stats/training_status.py
[warning] 51-51: src/garth/stats/training_status.py#L51
Added line #L51 was not covered by tests
[warning] 55-55: src/garth/stats/training_status.py#L55
Added line #L55 was not covered by tests
[warning] 59-59: src/garth/stats/training_status.py#L59
Added line #L59 was not covered by tests
[warning] 65-65: src/garth/stats/training_status.py#L65
Added line #L65 was not covered by tests
[warning] 74-74: src/garth/stats/training_status.py#L74
Added line #L74 was not covered by tests
[warning] 79-79: src/garth/stats/training_status.py#L79
Added line #L79 was not covered by tests
[warning] 111-111: src/garth/stats/training_status.py#L111
Added line #L111 was not covered by tests
[warning] 126-126: src/garth/stats/training_status.py#L126
Added line #L126 was not covered by tests
🪛 LanguageTool
CLAUDE.md
[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)
🔇 Additional comments (5)
src/garth/__init__.py (1)
10-16: Public exports look correctThe new training-status classes are imported and will be exposed to users, matching the additions in
stats/__init__.py. LGTM.src/garth/stats/__init__.py (1)
8-14: Exports consistent across package
DailyTrainingStatus,WeeklyTrainingStatus, andMonthlyTrainingStatusare now publicly available here and at package root. Nice consistency.Also applies to: 22-26
tests/stats/test_training_status.py (1)
13-22: LGTM! Well-structured test for daily training status.The test correctly validates the basic functionality of retrieving a single day's training status data.
src/garth/stats/training_status.py (2)
14-40: Appropriate design for API data model.The large number of attributes is justified as this is a data transfer object matching the API response structure. The use of optional fields provides flexibility for partial responses.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 15-15: Too many instance attributes (24/7)
(R0902)
96-138: Well-implemented pagination logic.The recursive approach to handle pagination is clean and effective. The method properly handles date calculations and API responses.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 111-111: src/garth/stats/training_status.py#L111
Added line #L111 was not covered by tests
[warning] 126-126: src/garth/stats/training_status.py#L126
Added line #L126 was not covered by tests
- Test error cases in _extract_training_data method - Test error cases in list method for non-dict responses - Test pagination when first page is empty - Achieve 100% test coverage for training status module 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Split single training_status.py into separate daily/weekly/monthly modules - Use existing Stats base class instead of duplicating functionality - Follow body battery pattern for directory structure - Add comprehensive error handling tests achieving 100% coverage - Fix import paths and typing issues for the new structure 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Remove problematic list[dict] return type annotations that cause issues with mypy type checking in CI environments.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/garth/stats/training_status/daily.py (1)
38-42:_page_sizeis dead code in the daily implementation
_page_sizeis never referenced inDailyTrainingStatus.
Because the/latest/{end}endpoint always returns a single day, the
attribute is misleading noise.- _page_size: ClassVar[int] = 28Unless you plan to add real pagination, drop the constant to avoid
confusion.tests/stats/test_training_status.py (1)
225-233: Unreachableelseblock – small readability clean-upThe
elseafter areturnis 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 nitMissing 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)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
CLAUDE.md(1 hunks)src/garth/stats/training_status/__init__.py(1 hunks)src/garth/stats/training_status/daily.py(1 hunks)src/garth/stats/training_status/monthly.py(1 hunks)src/garth/stats/training_status/weekly.py(1 hunks)tests/stats/test_training_status.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/garth/stats/training_status/init.py
🧰 Additional context used
📓 Path-based instructions (1)
`tests/**`: - test functions shouldn't have a return type hint - it's ok to use `assert` instead of `pytest.assume()`
tests/**: - test functions shouldn't have a return type hint
- it's ok to use
assertinstead ofpytest.assume()
tests/stats/test_training_status.py
🪛 Pylint (3.3.7)
src/garth/stats/training_status/monthly.py
[refactor] 13-13: Too many instance attributes (23/7)
(R0902)
src/garth/stats/training_status/weekly.py
[refactor] 13-13: Too many instance attributes (23/7)
(R0902)
src/garth/stats/training_status/daily.py
[refactor] 13-13: Too many instance attributes (23/7)
(R0902)
tests/stats/test_training_status.py
[refactor] 227-232: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🪛 LanguageTool
CLAUDE.md
[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)
🔇 Additional comments (1)
src/garth/stats/training_status/weekly.py (1)
55-65: Pagination order looks reversed
pagefetches the most-recent 52 weeks;remaining_pagethen fetches
older data and you returnremaining_page + page, meaning the list is
oldest-to-newest. Confirm this is the intended order (other
Stats.list()implementations return newest-to-oldest).
- Extend Stats base class with _parse_response() hook method - Remove 101 lines of duplicated code (33% reduction) from training status - Eliminate duplicate pagination, HTTP client, and formatting logic - Training status classes now only implement custom response parsing - Maintain 100% backward compatibility and test coverage - Follow Template Method pattern for better maintainability This refactoring demonstrates strategic use of inheritance to eliminate code duplication while maintaining flexibility for custom API response parsing in the training status modules. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
src/garth/stats/_base.py (1)
28-45:⚠️ Potential issueMonthly periods are computed in weeks – date ranges are wrong
Theperiod_typefallback 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_sizeweeks 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 fromdatetime.timedelta
(which has no months) todateutil.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.
♻️ Duplicate comments (2)
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
_pathignoresperiod, causing odd behaviour forperiod>1
/trainingstatus/latest/{end}can only ever return one day. When callers pass
period > 1,Stats.listrepeatedly hits the same latest endpoint with
earlierenddates, 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 == 1is supported.
🧹 Nitpick comments (2)
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_dirsindependent of the raw API payload.tests/stats/test_training_status.py (1)
225-233: Minor style nit –elseafterreturnis redundantif 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_responseRemoving the
elseshortens 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)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/garth/stats/_base.py(1 hunks)src/garth/stats/training_status/daily.py(1 hunks)src/garth/stats/training_status/monthly.py(1 hunks)src/garth/stats/training_status/weekly.py(1 hunks)tests/stats/test_training_status.py(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`tests/**`: - test functions shouldn't have a return type hint - it's ok to use `assert` instead of `pytest.assume()`
tests/**: - test functions shouldn't have a return type hint
- it's ok to use
assertinstead ofpytest.assume()
tests/stats/test_training_status.py
🧬 Code Graph Analysis (2)
src/garth/stats/training_status/weekly.py (3)
src/garth/stats/_base.py (2)
Stats(12-68)_parse_response(57-68)src/garth/stats/training_status/monthly.py (1)
_parse_response(40-70)src/garth/stats/training_status/daily.py (1)
_parse_response(40-66)
src/garth/stats/_base.py (5)
src/garth/http.py (1)
connectapi(186-192)src/garth/stats/training_status/monthly.py (1)
_parse_response(40-70)src/garth/stats/training_status/daily.py (1)
_parse_response(40-66)src/garth/stats/training_status/weekly.py (1)
_parse_response(40-70)src/garth/utils.py (1)
camel_to_snake_dict(17-33)
🪛 Pylint (3.3.7)
src/garth/stats/training_status/weekly.py
[refactor] 9-9: Too many instance attributes (23/7)
(R0902)
src/garth/stats/training_status/daily.py
[refactor] 9-9: Too many instance attributes (23/7)
(R0902)
src/garth/stats/training_status/monthly.py
[refactor] 9-9: Too many instance attributes (23/7)
(R0902)
tests/stats/test_training_status.py
[refactor] 227-232: Unnecessary "else" after "return", remove the "else" and de-indent the code inside it
(R1705)
🔇 Additional comments (1)
src/garth/stats/training_status/monthly.py (1)
34-38: Pagination size (12) is correct butStats.liststill works in weeks
Even with_page_size = 12, the base class bug (see earlier comment) will
subtract 12 weeks, not 12 months. Ensure the base-class fix is applied, or
MonthlyTrainingStatus.list(period>12)will request an 84-day window instead
of a 1-year window.
| # 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: | ||
| if isinstance(entry, dict): | ||
| # Flatten the acuteTrainingLoadDTO data | ||
| acute_load = entry.pop("acuteTrainingLoadDTO", {}) | ||
| if isinstance(acute_load, dict): | ||
| entry.update(acute_load) | ||
| result.append(entry) | ||
| return result |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Only the first device’s data is returned – multi-device accounts lose information
for device_data in report_data.values(): return … exits after the first
iteration; if a user owns multiple watches, the remaining devices’ weekly
stats are dropped.
Consider aggregating all device_data lists:
- for device_data in report_data.values():
- if isinstance(device_data, list):
- result = []
- for entry in device_data:
+ result: list[dict] = []
+ for device_data in report_data.values():
+ if isinstance(device_data, list):
+ for entry in device_data:
…
- return result
-
- return []
+ return resultReturning a complete set avoids silent data loss.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 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: | |
| if isinstance(entry, dict): | |
| # Flatten the acuteTrainingLoadDTO data | |
| acute_load = entry.pop("acuteTrainingLoadDTO", {}) | |
| if isinstance(acute_load, dict): | |
| entry.update(acute_load) | |
| result.append(entry) | |
| return result | |
| # Get all devices’ data (was only returning the first device) | |
| result: list[dict] = [] | |
| for device_data in report_data.values(): | |
| if isinstance(device_data, list): | |
| for entry in device_data: | |
| if isinstance(entry, dict): | |
| # Flatten the acuteTrainingLoadDTO data | |
| acute_load = entry.pop("acuteTrainingLoadDTO", {}) | |
| if isinstance(acute_load, dict): | |
| entry.update(acute_load) | |
| result.append(entry) | |
| return result |
🤖 Prompt for AI Agents
In src/garth/stats/training_status/weekly.py around lines 57 to 68, the code
returns data from only the first device by exiting the loop early, causing loss
of data for users with multiple devices. Modify the code to aggregate the
processed entries from all devices into a single list before returning, ensuring
that data from every device in report_data.values() is included in the final
result.
- Change since_date from str to date type - Change timestamp from int to datetime type - Add datetime type validation in tests - Update README examples to show proper datetime objects - Resolve CodeRabbit comments on dict() usage and test structure 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
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 earlyreturndrops data for users who own multiple devices. Please aggregate entries from alldevice_datalists 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 resultsrc/garth/stats/training_status/monthly.py (1)
35-39: 🛠️ Refactor suggestion
⚠️ Potential issuePeriod maths are still based on ISO-weeks → monthly ranges are wrong
Because
Stats.list()defaults toperiod_type = "weeks"for every path that is notdaily, calls such asMonthlyTrainingStatus.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 todateutil.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:periodargument still functionally ignored – comment from previous review applies
DailyTrainingStatusrelies on an endpoint that always returns a single
day’s record, soStats.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.)
🧹 Nitpick comments (3)
src/garth/sso.py (1)
159-159: Minor nit – optional empty body
data = {}works, but whenoauth1.mfa_tokenis falsy you could passNoneto 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 NoneNot 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_responsein 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 copyThe extractor correctly flattens
acuteTrainingLoadDTOand copes with unexpected shapes.
Minor:entry.pop("acuteTrainingLoadDTO", {})mutates the original API dictionary; useentry.getand merge into a new dict to avoid side-effects if the caller reuses the raw response elsewhere.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)src/garth/sso.py(3 hunks)src/garth/stats/training_status/daily.py(1 hunks)src/garth/stats/training_status/monthly.py(1 hunks)src/garth/stats/training_status/weekly.py(1 hunks)tests/stats/test_training_status.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- tests/stats/test_training_status.py
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/garth/stats/training_status/daily.py (3)
src/garth/stats/_base.py (2)
Stats(12-68)_parse_response(57-68)src/garth/stats/training_status/weekly.py (1)
_parse_response(41-71)src/garth/stats/training_status/monthly.py (1)
_parse_response(41-71)
src/garth/stats/training_status/monthly.py (3)
src/garth/stats/_base.py (2)
Stats(12-68)_parse_response(57-68)src/garth/stats/training_status/weekly.py (1)
_parse_response(41-71)src/garth/stats/training_status/daily.py (1)
_parse_response(41-67)
src/garth/stats/training_status/weekly.py (3)
src/garth/stats/_base.py (2)
Stats(12-68)_parse_response(57-68)src/garth/stats/training_status/daily.py (1)
_parse_response(41-67)src/garth/stats/training_status/monthly.py (1)
_parse_response(41-71)
🪛 Pylint (3.3.7)
src/garth/stats/training_status/daily.py
[refactor] 10-10: Too many instance attributes (23/7)
(R0902)
src/garth/stats/training_status/monthly.py
[refactor] 10-10: Too many instance attributes (23/7)
(R0902)
src/garth/stats/training_status/weekly.py
[refactor] 10-10: Too many instance attributes (23/7)
(R0902)
🔇 Additional comments (4)
src/garth/sso.py (3)
75-79: Consistent dict-literal usage looks goodSwitching from
dict()to literal{}is more idiomatic and a hair faster; no behavioural change introduced.
82-88: OverridinggauthHostkey is unchanged – all goodThe merge correctly overrides the earlier
gauthHostvalue withSSO_EMBED. Behaviour matches previous code.
109-114: Style-only change confirmedThe POST payload is identical to the earlier implementation; nothing else to flag.
src/garth/stats/training_status/monthly.py (1)
9-34: Large dataclass field set is expected – ignore pylint R0902This dataclass is a straight mirror of Garmin’s payload; the high attribute count is acceptable and suppressing the pylint warning is fine.
🧰 Tools
🪛 Pylint (3.3.7)
[refactor] 10-10: Too many instance attributes (23/7)
(R0902)
| _path: ClassVar[str] = ( | ||
| "/mobile-gateway/usersummary/trainingstatus/latest/{end}" | ||
| ) | ||
| _page_size: ClassVar[int] = 28 |
There was a problem hiding this comment.
_path lacks “daily” – date math is done in weeks, breaking multi-day queries
Stats.list() decides whether to subtract timedelta(days=…) or timedelta(weeks=…) by checking if the substring "daily" is present in cls._path.
Because this path is …/trainingstatus/latest/{end} (no “daily”), period_type is set to "weeks", so:
start = end - timedelta(weeks=period-1) # ← wrong unitDailyTrainingStatus.list(end, period=3) therefore fetches the wrong
dates.
Fix options:
-_path: ClassVar[str] = (
- "/mobile-gateway/usersummary/trainingstatus/latest/{end}"
-)
+# Keep behaviour aligned with the heuristic in Stats.list()
+_path: ClassVar[str] = (
+ "/mobile-gateway/usersummary/trainingstatus/daily/{end}"
+)or override list() in this subclass and force period_type = "days".
Either way, ensure the unit test covers period > 1.
🤖 Prompt for AI Agents
In src/garth/stats/training_status/daily.py around lines 35 to 38, the _path
string lacks the substring "daily", causing Stats.list() to use weeks instead of
days for date calculations, which breaks multi-day queries. To fix this, update
the _path to include "daily" so the period_type is correctly set to "days", or
alternatively override the list() method in this subclass to explicitly set
period_type to "days". Also, add or update unit tests to cover cases where
period > 1 to verify correct behavior.
| @classmethod | ||
| def _parse_response(cls, response): | ||
| """Extract training data from the daily API response structure.""" | ||
| if not isinstance(response, dict): | ||
| return [] | ||
|
|
||
| data_section = response.get("mostRecentTrainingStatus", {}) | ||
| if not isinstance(data_section, dict): | ||
| return [] | ||
|
|
||
| payload = data_section.get("payload", {}) | ||
| if not isinstance(payload, dict): | ||
| return [] | ||
|
|
||
| latest_data = payload.get("latestTrainingStatusData", {}) | ||
| if not isinstance(latest_data, dict): | ||
| return [] | ||
|
|
||
| # Get the first device's data (assumes single device for now) | ||
| for device_data in latest_data.values(): | ||
| if isinstance(device_data, dict): | ||
| # Flatten the acuteTrainingLoadDTO data | ||
| acute_load = device_data.pop("acuteTrainingLoadDTO", {}) | ||
| if isinstance(acute_load, dict): | ||
| device_data.update(acute_load) | ||
| return [device_data] | ||
|
|
||
| return [] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Parser drops data from secondary devices – only the first device makes it through
_parse_response() stops after the first value in latestTrainingStatusData.
If the user owns multiple devices (common with Garmin), their additional
records are silently discarded.
Minimal change:
- for device_data in latest_data.values():
- if isinstance(device_data, dict):
- …
- return [device_data]
- return []
+ result: list[dict] = []
+ for device_data in latest_data.values():
+ if isinstance(device_data, dict):
+ acute_load = device_data.pop("acuteTrainingLoadDTO", {}) or {}
+ if isinstance(acute_load, dict):
+ device_data.update(acute_load)
+ result.append(device_data)
+ return resultThis keeps the method contract (list[dict]) while preserving all device
entries.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @classmethod | |
| def _parse_response(cls, response): | |
| """Extract training data from the daily API response structure.""" | |
| if not isinstance(response, dict): | |
| return [] | |
| data_section = response.get("mostRecentTrainingStatus", {}) | |
| if not isinstance(data_section, dict): | |
| return [] | |
| payload = data_section.get("payload", {}) | |
| if not isinstance(payload, dict): | |
| return [] | |
| latest_data = payload.get("latestTrainingStatusData", {}) | |
| if not isinstance(latest_data, dict): | |
| return [] | |
| # Get the first device's data (assumes single device for now) | |
| for device_data in latest_data.values(): | |
| if isinstance(device_data, dict): | |
| # Flatten the acuteTrainingLoadDTO data | |
| acute_load = device_data.pop("acuteTrainingLoadDTO", {}) | |
| if isinstance(acute_load, dict): | |
| device_data.update(acute_load) | |
| return [device_data] | |
| return [] | |
| @classmethod | |
| def _parse_response(cls, response): | |
| """Extract training data from the daily API response structure.""" | |
| if not isinstance(response, dict): | |
| return [] | |
| data_section = response.get("mostRecentTrainingStatus", {}) | |
| if not isinstance(data_section, dict): | |
| return [] | |
| payload = data_section.get("payload", {}) | |
| if not isinstance(payload, dict): | |
| return [] | |
| latest_data = payload.get("latestTrainingStatusData", {}) | |
| if not isinstance(latest_data, dict): | |
| return [] | |
| result: list[dict] = [] | |
| for device_data in latest_data.values(): | |
| if isinstance(device_data, dict): | |
| acute_load = device_data.pop("acuteTrainingLoadDTO", {}) or {} | |
| if isinstance(acute_load, dict): | |
| device_data.update(acute_load) | |
| result.append(device_data) | |
| return result |
🤖 Prompt for AI Agents
In src/garth/stats/training_status/daily.py between lines 40 and 67, the
_parse_response method currently returns only the first device's data from
latestTrainingStatusData, discarding any additional devices. To fix this, modify
the method to iterate over all device_data entries in latestTrainingStatusData,
flatten each acuteTrainingLoadDTO into its device_data, and collect all
processed device_data dictionaries into a list that is returned at the end,
ensuring all devices' data are preserved and returned as a list of dicts.
🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
- Add _period_type class variable to Stats base class for explicit override - Set DailyTrainingStatus._period_type = "days" to fix date calculations - Add test to verify period type is correctly applied - Regenerate VCR cassettes with correct API calls - Update README examples to use consistent parameter format - Maintains backward compatibility with existing stats classes Resolves CodeRabbit potential issue: daily training status was using weeks instead of days for date math due to path not containing "daily" keyword. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>
Summary
API Endpoints Added
/mobile-gateway/usersummary/trainingstatus/latest/{date}/mobile-gateway/usersummary/trainingstatus/weekly/{start}/{end}/mobile-gateway/usersummary/trainingstatus/monthly/{start}/{end}Implementation Details
Test Coverage
Closes #129
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests