Add additional metrics - Hill + Endurance score, Morning Training Readiness#135
Conversation
|
Warning Rate limit exceeded@matin has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 14 minutes and 46 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughAdds two new data models, GarminScoresData and MorningTrainingReadinessData, exposes them in package exports, adds README examples (duplicated), and includes tests for their get() and list() methods. No runtime API signatures were changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as User / Test
participant SDK as garth Data Class
participant API as Garmin API
Caller->>SDK: GarminScoresData.get(date)
SDK->>API: GET /metrics-service/metrics/hillscore/{date}
API-->>SDK: hill payload (camelCase)
SDK->>API: GET /metrics-service/metrics/endurancescore/{date}
API-->>SDK: endurance payload (camelCase)
SDK->>SDK: camel_to_snake_dict, rename keys, merge datasets
SDK-->>Caller: GarminScoresData instance or None
Caller->>SDK: MorningTrainingReadinessData.get(date)
SDK->>API: GET /metrics-service/metrics/trainingreadiness/{date}
API-->>SDK: readiness payload (list)
SDK->>SDK: select entry with inputContext == "AFTER_WAKEUP_RESET"
SDK->>SDK: camel_to_snake_dict, construct dataclass
SDK-->>Caller: MorningTrainingReadinessData instance or None
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
src/garth/data/garmin_scores.py (2)
38-46: Consider API call efficiency and error handlingThe method makes two separate API calls which could be inefficient, but this may be necessary due to the API design. The error handling returns
Noneif either endpoint fails, which is reasonable but could be more granular.Consider if both endpoints are always required or if partial data could be useful in some cases.
60-65: Complex field renaming logic - consider adding commentsThe dictionary comprehension for renaming endurance classification fields is complex but appears correct. Consider adding a comment to explain the logic for better maintainability.
+ # Rename classification fields to include 'endurance_classification' prefix data_endurance_score = { ("endurance_classification" + key[len("classification") :]) if key.startswith("classification") else key: value for key, value in data_endurance_score.items() }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
tests/data/cassettes/test_garmin_scores_data_get.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_garmin_scores_data_list.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_morning_training_readiness_data_get.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_morning_training_readiness_data_list.yamlis excluded by!tests/**/cassettes/**
📒 Files selected for processing (7)
README.md(1 hunks)src/garth/__init__.py(2 hunks)src/garth/data/__init__.py(2 hunks)src/garth/data/garmin_scores.py(1 hunks)src/garth/data/morning_training_readiness.py(1 hunks)tests/data/test_garmin_scores.py(1 hunks)tests/data/test_morning_training_readiness.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()
⚙️ Source: CodeRabbit Configuration File
List of files the instruction was applied to:
tests/data/test_garmin_scores.pytests/data/test_morning_training_readiness.py
🧠 Learnings (2)
tests/data/test_garmin_scores.py (1)
Learnt from: matin
PR: matin/garth#76
File: tests/test_http.py:109-109
Timestamp: 2024-12-07T14:07:11.531Z
Learning: In Python test files (e.g., `tests/test_http.py`), top-level test functions should be separated by two blank lines, as per PEP 8 style guidelines.
tests/data/test_morning_training_readiness.py (1)
Learnt from: matin
PR: matin/garth#76
File: tests/test_http.py:109-109
Timestamp: 2024-12-07T14:07:11.531Z
Learning: In Python test files (e.g., `tests/test_http.py`), top-level test functions should be separated by two blank lines, as per PEP 8 style guidelines.
🧬 Code Graph Analysis (2)
src/garth/__init__.py (3)
src/garth/data/garmin_scores.py (1)
GarminScoresData(14-74)src/garth/data/hrv.py (1)
HRVData(39-68)src/garth/data/morning_training_readiness.py (1)
MorningTrainingReadinessData(15-69)
src/garth/data/morning_training_readiness.py (3)
src/garth/utils.py (1)
camel_to_snake_dict(17-33)src/garth/data/_base.py (1)
Data(15-47)src/garth/http.py (2)
Client(19-244)connectapi(186-192)
🔇 Additional comments (13)
src/garth/__init__.py (2)
4-6: LGTM - Imports follow existing patternsThe new imports are correctly placed and follow the established alphabetical ordering within the import block.
36-38: LGTM - Exports properly added to allThe new classes are correctly added to the
__all__export list in alphabetical order, maintaining consistency with existing patterns.README.md (1)
908-1068: LGTM - Comprehensive documentation for new featuresThe new documentation sections are well-structured and follow the existing style patterns. The examples demonstrate both single-date and multi-date usage scenarios with detailed sample outputs, which will be helpful for users implementing these features.
src/garth/data/__init__.py (1)
6-8: LGTM - Consistent with existing module patternsThe new exports and imports are correctly added following the established alphabetical ordering and import structure patterns.
Also applies to: 21-23
src/garth/data/garmin_scores.py (2)
14-29: LGTM - Well-structured dataclass definitionThe dataclass fields are comprehensive and well-named, covering all the metrics from both hill and endurance score endpoints. The field names are descriptive and follow Python naming conventions.
71-74: LGTM - Consistent list method implementationThe list method correctly delegates to the parent class and sorts by calendar_date, which is consistent with other data classes in the codebase.
tests/data/test_garmin_scores.py (2)
9-24: LGTM - Comprehensive test coverage for get() methodThe test correctly verifies both positive and negative cases:
- Tests successful data retrieval with assertions on key fields
- Tests the case where no data is available (returns None)
- Follows the established testing patterns with
pytest.mark.vcrandauthed_clientfixture
26-34: LGTM - Appropriate test for list() methodThe test verifies the list functionality correctly:
- Tests with appropriate parameters (days=2, max_workers=1)
- Verifies the returned data length matches expected days
- Checks that the last entry matches the expected end date
- Uses single worker to ensure deterministic test execution
tests/data/test_morning_training_readiness.py (1)
26-34: Test implementation looks good.The test properly validates the list method functionality, checking both the returned length and the last entry's date.
src/garth/data/morning_training_readiness.py (4)
1-12: Imports and setup look good.The imports are appropriately scoped and the module structure follows the established pattern in the codebase.
14-35: Well-structured dataclass with comprehensive field definitions.The dataclass appropriately captures all the morning training readiness metrics with correct type annotations. The field names follow Python naming conventions.
37-64: Robust implementation of the get method.The method properly:
- Handles the case when no data is returned from the API
- Filters for the specific "AFTER_WAKEUP_RESET" context to isolate morning readiness data
- Uses the established
camel_to_snake_dictutility for field conversion- Follows the pattern established in other data classes
66-69: Good override of the list method.The method appropriately sorts the results by calendar_date, which provides a consistent and logical ordering for time-series data.
vactomas
force-pushed
the
feature/add-additional-garmin-scores
branch
from
840a25b to
5d23551
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #135 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 45 49 +4
Lines 1911 2047 +136
==========================================
+ Hits 1911 2047 +136
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:
|
|
@vactomas both of your PRs have the same issue. let's put you could also mock the response, but don't worry about it. let's get these PRs merged |
vactomas
force-pushed
the
feature/add-additional-garmin-scores
branch
from
5d23551 to
c3b55dd
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/garth/data/morning_training_readiness.py (2)
45-49: Defensive type check on connectapi resultconnectapi() is annotated to return Dict[str, Any] | None but this endpoint returns a list; guard to avoid unexpected crashes if a non-list slips through.
if not raw_data: return None + if not isinstance(raw_data, list): + # Defensive: unexpected shape (e.g., error object) + return None data = cast(list[dict[str, Any]], raw_data)
41-44: Normalize date formattingBe explicit with ISO formatting when day is a date.
- path = f"/metrics-service/metrics/trainingreadiness/{day}" + day_str = day.isoformat() if isinstance(day, date) else day + path = f"/metrics-service/metrics/trainingreadiness/{day_str}"tests/data/test_morning_training_readiness.py (1)
33-34: Reduce potential flakiness in length assertionDays without readiness would shrink the list; assert non-empty and keep the end-date check.
- assert len(morning_training_readiness) == days + assert len(morning_training_readiness) >= 1 assert morning_training_readiness[-1].calendar_date == end
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (2)
tests/data/cassettes/test_morning_training_readiness_data_get.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_morning_training_readiness_data_list.yamlis excluded by!tests/**/cassettes/**
📒 Files selected for processing (5)
README.md(1 hunks)src/garth/__init__.py(2 hunks)src/garth/data/__init__.py(2 hunks)src/garth/data/morning_training_readiness.py(1 hunks)tests/data/test_morning_training_readiness.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/garth/data/init.py
- src/garth/init.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/**
⚙️ CodeRabbit configuration file
tests/**: - test functions shouldn't have a return type hint
- it's ok to use
assertinstead ofpytest.assume()
Files:
tests/data/test_morning_training_readiness.py
🧬 Code graph analysis (2)
tests/data/test_morning_training_readiness.py (3)
src/garth/data/morning_training_readiness.py (2)
MorningTrainingReadinessData(15-70)get(38-65)src/garth/http.py (1)
Client(19-244)tests/conftest.py (3)
vcr(79-82)authed_client(65-75)client(21-22)
src/garth/data/morning_training_readiness.py (3)
src/garth/utils.py (1)
camel_to_snake_dict(17-33)src/garth/data/_base.py (1)
Data(15-47)src/garth/http.py (2)
Client(19-244)connectapi(186-192)
🔇 Additional comments (4)
src/garth/data/morning_training_readiness.py (2)
68-70: Sorted output is goodStable ordering by calendar_date is appropriate for consumers.
60-63: Verified coverage pragma placement Only the requested# pragma: no coverappears insrc/garth/data/morning_training_readiness.py; no such pragmas are present (or needed) insrc/garth/data/garmin_scores.py.tests/data/test_morning_training_readiness.py (2)
9-18: Happy-path coverage looks goodCovers existence and key fields with VCR; fixture usage is clear.
20-23: None-path check is goodValidates 204/empty scenarios without extra mocking.
matin
force-pushed
the
feature/add-additional-garmin-scores
branch
from
c3b55dd to
f5fb2d9
Compare
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/garth/data/garmin_scores.py:
- Around line 45-46: Add a coverage exclusion for the hard-to-test None return
by appending "# pragma: no cover" to the `return None` line in
src/garth/data/garmin_scores.py (the branch that triggers when `not
data_hill_score or not data_endurance_score`), mirroring the pattern used in
morning_training_readiness.py; ensure only the `return None` statement is
annotated so other logic remains covered.
In @src/garth/data/morning_training_readiness.py:
- Around line 51-58: The generator for morning_readiness_data currently indexes
entry["inputContext"], which can raise KeyError if the key is missing; update
the filter in the next(...) call to use entry.get("inputContext") ==
"AFTER_WAKEUP_RESET" (or compare against None-safe value) so missing keys do not
raise, keeping the variable name morning_readiness_data and the surrounding
logic unchanged.
🧹 Nitpick comments (1)
src/garth/data/garmin_scores.py (1)
60-65: Consider simplifying the classification field renaming.The dictionary comprehension works but is somewhat dense. A more explicit approach might improve readability.
♻️ Alternative approach
- data_endurance_score = { - ("endurance_classification" + key[len("classification") :]) - if key.startswith("classification") - else key: value - for key, value in data_endurance_score.items() - } + # Prefix classification fields with 'endurance_' + renamed = {} + for key, value in data_endurance_score.items(): + if key.startswith("classification"): + renamed[f"endurance_{key}"] = value + else: + renamed[key] = value + data_endurance_score = renamed
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
tests/data/cassettes/test_garmin_scores_data_get.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_garmin_scores_data_list.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_morning_training_readiness_data_get.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_morning_training_readiness_data_list.yamlis excluded by!tests/**/cassettes/**
📒 Files selected for processing (7)
README.mdsrc/garth/__init__.pysrc/garth/data/__init__.pysrc/garth/data/garmin_scores.pysrc/garth/data/morning_training_readiness.pytests/data/test_garmin_scores.pytests/data/test_morning_training_readiness.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/garth/init.py
- src/garth/data/init.py
- README.md
- tests/data/test_morning_training_readiness.py
🧰 Additional context used
📓 Path-based instructions (1)
tests/**
⚙️ CodeRabbit configuration file
tests/**: - test functions shouldn't have a return type hint
- it's ok to use
assertinstead ofpytest.assume()
Files:
tests/data/test_garmin_scores.py
🧬 Code graph analysis (3)
tests/data/test_garmin_scores.py (1)
src/garth/data/garmin_scores.py (2)
GarminScoresData(14-74)get(32-69)
src/garth/data/garmin_scores.py (3)
src/garth/utils.py (1)
camel_to_snake_dict(17-33)src/garth/data/_base.py (1)
Data(15-47)src/garth/http.py (2)
Client(19-245)connectapi(186-192)
src/garth/data/morning_training_readiness.py (4)
src/garth/utils.py (1)
camel_to_snake_dict(17-33)src/garth/data/_base.py (1)
Data(15-47)tests/conftest.py (1)
client(21-22)src/garth/http.py (2)
Client(19-245)connectapi(186-192)
🔇 Additional comments (12)
src/garth/data/morning_training_readiness.py (4)
1-12: LGTM: Imports are clean and appropriate.All necessary imports are present and properly organized.
14-35: LGTM: Data model structure is well-defined.The dataclass fields are comprehensive and properly typed for morning training readiness metrics.
37-65: LGTM: get() method implementation is solid.The method correctly fetches data, filters for morning readiness entries, converts keys, and handles missing data gracefully.
67-70: LGTM: list() method delegates correctly.Proper delegation to the base implementation with appropriate sorting by calendar_date.
tests/data/test_garmin_scores.py (3)
1-7: LGTM: Imports are appropriate.Clean imports for testing GarminScoresData functionality.
9-23: LGTM: Well-structured test for get() method.The test correctly validates both successful data retrieval and the None case when data is unavailable. Proper use of vcr for HTTP recording.
26-34: LGTM: Comprehensive list() method test.Good test coverage with max_workers=1 for deterministic execution and validation of both length and ordering.
src/garth/data/garmin_scores.py (5)
1-10: LGTM: Imports are appropriate.Clean imports following the same pattern as MorningTrainingReadinessData.
13-29: LGTM: Data model is well-structured.The dataclass comprehensively captures Garmin score metrics with proper type annotations.
48-59: LGTM: Field transformation logic is clear.The camelCase to snake_case conversion and field renaming (overall_score → hill_score, etc.) are explicit and maintainable.
67-69: LGTM: Data merging is straightforward.Dictionary unpacking cleanly combines hill and endurance score data.
71-74: LGTM: list() method follows the established pattern.Proper delegation with sorting by calendar_date, consistent with MorningTrainingReadinessData.
| if not data_hill_score or not data_endurance_score: | ||
| return None |
There was a problem hiding this comment.
Add pragma: no cover for the None return path.
This mirrors the pattern in morning_training_readiness.py line 60 and aligns with the reviewer's request to add pragma coverage for difficult-to-test None returns.
🛠️ Proposed fix
+ # pragma: no cover
if not data_hill_score or not data_endurance_score:
return NoneBased on PR objectives feedback from reviewer matin.
📝 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.
| if not data_hill_score or not data_endurance_score: | |
| return None | |
| # pragma: no cover | |
| if not data_hill_score or not data_endurance_score: | |
| return None |
🤖 Prompt for AI Agents
In @src/garth/data/garmin_scores.py around lines 45 - 46, Add a coverage
exclusion for the hard-to-test None return by appending "# pragma: no cover" to
the `return None` line in src/garth/data/garmin_scores.py (the branch that
triggers when `not data_hill_score or not data_endurance_score`), mirroring the
pattern used in morning_training_readiness.py; ensure only the `return None`
statement is annotated so other logic remains covered.
|
@vactomas sorry it took so long to get to your PRs. working through them now thanks for the great work! |
2 participants
Hi, matin,
I've added a couple of additional metrics, which can be obtained from the Garmin API. I believe they could be of use to people, who process the data themselves.
Let me know, what you think!
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.