Add an option to list daily summary#132
Conversation
|
Warning Rate limit exceeded@matin has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 15 minutes and 27 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 (1)
WalkthroughAdds a new Pydantic dataclass DailySummary with a classmethod to fetch daily metrics from the external connect API, exposes it via package exports, adds tests for fetching/listing daily summaries, and documents usage in README. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DailySummary
participant httpClient as http.Client
participant API
User->>DailySummary: DailySummary.get(day, client?)
alt no client provided
DailySummary->>httpClient: instantiate default client
end
DailySummary->>httpClient: connectapi("/wellness/dailySummary?calendarDate=day")
httpClient->>API: GET /wellness/dailySummary?calendarDate=day
API-->>httpClient: JSON response (camelCase keys)
httpClient-->>DailySummary: response dict
DailySummary->>DailySummary: camel_to_snake_dict -> validate -> instantiate
DailySummary-->>User: DailySummary instance or None
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/garth/__init__.py(2 hunks)src/garth/data/__init__.py(2 hunks)src/garth/data/daily_summary.py(1 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
src/garth/data/daily_summary.py
[refactor] 12-12: Too many instance attributes (27/7)
(R0902)
🔇 Additional comments (4)
src/garth/data/__init__.py (1)
6-6: LGTM! Proper API exposure.The changes correctly add
DailySummaryto the module's public API following established patterns for other data classes.Also applies to: 19-19
src/garth/__init__.py (1)
1-7: LGTM! Clean package-level integration.The changes properly integrate
DailySummaryinto the main package API with improved import formatting and correct alphabetical ordering in__all__.Also applies to: 34-34
src/garth/data/daily_summary.py (2)
1-8: LGTM! Well-organized imports.Import statements follow Python conventions with proper separation of standard library, third-party, and local imports.
11-40: Data class design is appropriate despite many fields.While static analysis flags 27 instance attributes as excessive, this is justified for a comprehensive daily health summary data class. All fields represent distinct health metrics that logically belong together, and the optional nature accommodates varying data availability from fitness devices.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/data/test_daily_summary.py (1)
9-27: Consider adding edge case tests and error handling validation.While the current tests cover the happy path well, consider adding tests for:
- Invalid date formats
- Network error scenarios
- Empty result sets
- Boundary conditions (e.g.,
days=0in list method)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/data/test_daily_summary.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/data/test_daily_summary.py
🔇 Additional comments (3)
tests/data/test_daily_summary.py (3)
1-6: Imports look good and follow conventions.The imports are well-organized and appropriate for the test functionality.
19-27: Test implementation looks solid with good coverage.The test effectively validates the list functionality with appropriate assertions for both count and date verification. The
max_workers=1parameter is a good practice for deterministic testing.
10-16: I’ll inspect the tests forDailySummary.listusage and then check the baseDataclass wherelistis likely defined:#!/usr/bin/env bash set -e # Show tests for DailySummary echo "--- tests/data/test_daily_summary.py ---" sed -n '1,200p' tests/data/test_daily_summary.py # Locate the file defining the base Data class data_file=$(rg -l "class Data" -n src/garth/data) echo "Data class implemented in: $data_file" # Show the Data class definition context echo -e "\n--- Data class definition ---" rg -n -A5 "class Data" "$data_file" # Show list() and get() signatures in Data echo -e "\n--- Data.list() signature ---" rg -n -C2 "def list" "$data_file" echo -e "\n--- Data.get() signature ---" rg -n -C2 "def get" "$data_file"
vactomas
force-pushed
the
feature/daily-summary
branch
from
bb93746 to
d0aab1f
Compare
|
@vactomas this is a good feature to add. ci is failing. could you check the tests? |
|
I see the problem, I have not generated VCR cassettes. I have a question, do I just run the tests against my account or is there some other way? I have locally temporarily set vcr |
vactomas
force-pushed
the
feature/daily-summary
branch
from
d0aab1f to
3d14c9c
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/garth/data/daily_summary.py (1)
45-56: Address the unresolved issues from previous review.The issues identified in the previous review regarding error handling and input validation remain unaddressed. As mentioned in the past review comments, this method needs:
- Input validation for string date format
- Proper error handling for API failures
- Descriptive error messages instead of bare assertions
🧹 Nitpick comments (1)
README.md (1)
910-910: Fix missing article for better readability.-Retrieve user's daily summary for given date. +Retrieve user's daily summary for a given date.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 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/daily_summary.py(1 hunks)tests/data/test_daily_summary.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/garth/init.py
- src/garth/data/init.py
- tests/data/test_daily_summary.py
🧰 Additional context used
🪛 LanguageTool
README.md
[uncategorized] ~910-~910: Possible missing article found.
Context: ...mary Retrieve user's daily summary for given date. ```python garth.DailySummary.get...
(AI_HYDRA_LEO_MISSING_A)
🪛 Pylint (3.3.7)
src/garth/data/daily_summary.py
[refactor] 12-12: Too many instance attributes (31/7)
(R0902)
🔇 Additional comments (2)
src/garth/data/daily_summary.py (1)
11-43: Well-structured data class for comprehensive health metrics.The
DailySummarydataclass effectively models the extensive daily health data from Garmin Connect. The use of optional fields withNonedefaults is appropriate since not all metrics may be available for every day.README.md (1)
908-1027: Excellent documentation following established patterns.The new "Daily Summary" section provides clear usage examples and comprehensive sample output that will help users understand the available metrics. The documentation structure is consistent with other sections in the README.
|
you have to log in, save your tokens and set the to generate the cassettes, this'll let you generate the tests against your account. all sensitive info should be masked, but i recommend double checking to cassettes before committing them |
|
I see, thanks. I have some trouble getting it to work, but I'll again later. Hopefully, I can get it to cooperate and get the cassettes ready |
vactomas
force-pushed
the
feature/daily-summary
branch
2 times, most recently
from
65d9bd4 to
4cb66bf
Compare
|
I have managed to get these cassettes to work locally. Hopefully, it will work even in CI |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/garth/data/daily_summary.py (1)
45-56: Duplicate: Error handling and input validation improvements needed.The error handling and input validation concerns raised in the previous review are still applicable to this implementation. The method would benefit from:
- Input validation for string date format
- Proper error handling around the API call
- Replacing the bare assertion with a descriptive error
🧹 Nitpick comments (1)
README.md (1)
910-910: Fix grammatical issue: missing article.The phrase "for given date" should include an article.
-Retrieve user's daily summary for given date. +Retrieve user's daily summary for a given date.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/data/cassettes/test_daily_summary_get.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_daily_summary_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/daily_summary.py(1 hunks)tests/data/test_daily_summary.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/garth/init.py
- tests/data/test_daily_summary.py
- src/garth/data/init.py
🧰 Additional context used
🧬 Code Graph Analysis (1)
src/garth/data/daily_summary.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)
🪛 LanguageTool
README.md
[uncategorized] ~910-~910: Possible missing article found.
Context: ...mary Retrieve user's daily summary for given date. ```python garth.DailySummary.get...
(AI_HYDRA_LEO_MISSING_A)
🪛 Pylint (3.3.7)
src/garth/data/daily_summary.py
[refactor] 12-12: Too many instance attributes (31/7)
(R0902)
🔇 Additional comments (3)
README.md (1)
908-1027: Excellent documentation for the new Daily Summary feature.The documentation follows the established pattern and provides comprehensive examples with clear sample output. The code examples demonstrate both single date retrieval and date range functionality effectively.
src/garth/data/daily_summary.py (2)
11-43: Well-structured data class for comprehensive health metrics.The
DailySummaryclass appropriately captures a wide range of daily health metrics with proper type annotations. While Pylint flags the high number of attributes (31), this is reasonable and expected for a comprehensive daily health summary that consolidates various health metrics into a single data structure.
1-8: Clean and appropriate imports.The imports are well-organized and include all necessary dependencies for the implementation.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 49 51 +2
Lines 2047 2113 +66
=========================================
+ Hits 2047 2113 +66
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:
|
|
I looked at the test coverage issue. There is a problem with the part: That said, if we try to pull a summary from time when the account wasn't used or has no data, it will still return non-empty return, but every single key is marked as None essentially. I believe the best way to solve this is to drop the if condition completely. What are your thoughts @matin? Also, is there anything else that needs to be solved? |
vactomas
force-pushed
the
feature/daily-summary
branch
from
4cb66bf to
bfec094
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/garth/data/daily_summary.py (1)
57-58: Replace assert with a typed error for clearer failures.Asserts can be stripped; raise a descriptive exception instead.
Apply this diff:
- assert isinstance(daily_summary, dict) + if not isinstance(daily_summary, dict): + raise TypeError(f"Expected dict from API, got {type(daily_summary)!r}")
🧹 Nitpick comments (5)
tests/data/test_daily_summary.py (2)
17-25: Assert ordering explicitly and rename plural for clarity.Make the order guarantee obvious and improve naming.
Apply this diff:
-from datetime import date +from datetime import date, timedelta @@ - daily_summary = DailySummary.list( + daily_summaries = DailySummary.list( end, days, client=authed_client, max_workers=1 ) - assert len(daily_summary) == days - assert daily_summary[0].calendar_date == end + assert len(daily_summaries) == days + assert [d.calendar_date for d in daily_summaries] == [end, end - timedelta(days=1)]
17-25: Optional: cover the “no content” branch without VCR.Add a lightweight unit test that monkeypatches
Client.connectapito returnNoneso theget()None-path is exercised.Example:
def test_daily_summary_get_none(monkeypatch, client: Client): monkeypatch.setattr(Client, "connectapi", lambda self, path, method="GET", **kw: None) assert DailySummary.get("2025-06-01", client=client) is Nonesrc/garth/data/daily_summary.py (3)
49-51: Build query string robustly and normalize date formatting.Avoid manual f-string concatenation; ensure consistent ISO formatting and proper encoding.
Apply this diff:
+from urllib.parse import urlencode @@ - path = f"/usersummary-service/usersummary/daily/?calendarDate={day}" + day_str = day.isoformat() if isinstance(day, date) else str(day) + qs = urlencode({"calendarDate": day_str}) + path = f"/usersummary-service/usersummary/daily/?{qs}"
53-55: Narrow the falsy check toNoneonly.The API often returns a dict with keys whose values are
None(truthy). Keep the 204/None case, don’t drop valid-but-empty days.Apply this diff:
- # pragma: no cover - if not daily_summary: - return None + # pragma: no cover + if daily_summary is None: + # 204 No Content + return NoneIf the endpoint can return
{}for no-activity days, confirm desired behavior (returnNonevs. construct with missing fields).
45-49: Minor: add a short docstring toget()for API/return semantics.Helps downstream users understand
Nonevs. populated objects.Example:
@classmethod def get(...): """Fetch a single day. Returns DailySummary or None (e.g., 204 No Content)."""
📜 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_daily_summary_get.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_daily_summary_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/daily_summary.py(1 hunks)tests/data/test_daily_summary.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/init.py
- src/garth/data/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_daily_summary.py
🧬 Code graph analysis (2)
tests/data/test_daily_summary.py (3)
src/garth/data/daily_summary.py (2)
DailySummary(12-59)get(46-59)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/daily_summary.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 (2)
tests/data/test_daily_summary.py (1)
9-14: Solid positive-path coverage for.get().Assertions are clear and stable with VCR. LGTM.
src/garth/data/daily_summary.py (1)
11-43: Schema looks good and maps cleanly to API fields.Field types and optionality align with expected payloads. LGTM.
matin
force-pushed
the
feature/daily-summary
branch
from
bfec094 to
e2091ec
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/garth/data/daily_summary.py:
- Line 58: Replace the fragile assertion with explicit error handling: check
that the parameter/variable daily_summary is an instance of dict and, if not,
raise a clear exception (e.g., TypeError or ValueError) with a descriptive
message mentioning daily_summary and expected type; update the surrounding
function or method (where daily_summary is validated) to use this explicit check
and consider logging the error before raising if appropriate.
🧹 Nitpick comments (1)
src/garth/data/daily_summary.py (1)
45-59: Consider adding explicit error handling for consistency and robustness.The
getmethod currently follows the pattern of other data models in the codebase (e.g.,WeightData,SleepData), which don't include try-except blocks. However,src/garth/data/body_battery/events.pydemonstrates a more robust approach: it wrapsclient.connectapi()in try-except with logging, gracefully handling potential network/API errors and data parsing failures.For
DailySummary.get(), explicit error handling would address:
- Network/API errors from
client.connectapi()- Schema validation errors when unpacking
**daily_summaryinto the Pydantic dataclassAdopting the error-handling pattern from
BodyBattery.eventswould align this method with best practices already demonstrated in the codebase.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/data/cassettes/test_daily_summary_get.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_daily_summary_list.yamlis excluded by!tests/**/cassettes/**
📒 Files selected for processing (5)
README.mdsrc/garth/__init__.pysrc/garth/data/__init__.pysrc/garth/data/daily_summary.pytests/data/test_daily_summary.py
🚧 Files skipped from review as they are similar to previous changes (4)
- src/garth/init.py
- src/garth/data/init.py
- tests/data/test_daily_summary.py
- README.md
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Usemake formatto auto-format Python source files using ruff
Usemake lintto lint Python source files (ruff format check, ruff check, mypy)
Files:
src/garth/data/daily_summary.py
🧬 Code graph analysis (1)
src/garth/data/daily_summary.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 (3)
src/garth/data/daily_summary.py (3)
1-8: LGTM: Imports are well-structured.All necessary imports are present and appropriately organized.
11-43: LGTM: Well-structured dataclass definition.The DailySummary model is properly designed with appropriate required and optional fields, and all fields are correctly typed.
53-55: Clarify the handling of missing data in API responses and consider explicit validation.The check
if not daily_summary:only catches None (HTTP 204) and empty dict/list responses, not dicts with missing required fields. SinceDailySummaryrequiresuser_profile_id: intandcalendar_date: date, if the API returns a dict without these fields, the check will pass but dataclass instantiation will fail with a Pydantic ValidationError.Consider one of these approaches:
If the API always returns a complete dict structure (even for inactive days): Remove the check as you suggested, since the dataclass validation will catch incomplete responses.
If the API can return None or incomplete responses: Keep the check but add explicit field validation—for example, verify that required fields exist after conversion:
if not daily_summary or not isinstance(daily_summary, dict): return None daily_summary = camel_to_snake_dict(daily_summary) if not all(k in daily_summary for k in ("user_profile_id", "calendar_date")): return NoneDocument the actual API behavior: Add a comment explaining when the API returns None vs. a dict, and what structure inactive days have.
Note: The
pragma: no coverannotation indicates this code path is untested, so verify against actual API responses.
| return None | ||
|
|
||
| daily_summary = camel_to_snake_dict(daily_summary) | ||
| assert isinstance(daily_summary, dict) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace assertion with proper error handling.
Using assert in production code is an anti-pattern because assertions can be disabled with Python's -O optimization flag.
♻️ Proposed fix
daily_summary = camel_to_snake_dict(daily_summary)
- assert isinstance(daily_summary, dict)
+ if not isinstance(daily_summary, dict):
+ raise TypeError(f"Expected dict from API, got {type(daily_summary).__name__}")
return cls(**daily_summary)🤖 Prompt for AI Agents
In @src/garth/data/daily_summary.py at line 58, Replace the fragile assertion
with explicit error handling: check that the parameter/variable daily_summary is
an instance of dict and, if not, raise a clear exception (e.g., TypeError or
ValueError) with a descriptive message mentioning daily_summary and expected
type; update the surrounding function or method (where daily_summary is
validated) to use this explicit check and consider logging the error before
raising if appropriate.
e2091ec to
7b2acd3
Compare
matin
force-pushed
the
feature/daily-summary
branch
from
7b2acd3 to
1e62a28
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/data/cassettes/test_daily_summary_get.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_daily_summary_list.yamlis excluded by!tests/**/cassettes/**
📒 Files selected for processing (5)
README.mdsrc/garth/__init__.pysrc/garth/data/__init__.pysrc/garth/data/daily_summary.pytests/data/test_daily_summary.py
🚧 Files skipped from review as they are similar to previous changes (4)
- README.md
- tests/data/test_daily_summary.py
- src/garth/init.py
- src/garth/data/init.py
🧰 Additional context used
📓 Path-based instructions (1)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Usemake formatto auto-format Python source files using ruff
Usemake lintto lint Python source files (ruff format check, ruff check, mypy)
Files:
src/garth/data/daily_summary.py
🧬 Code graph analysis (1)
src/garth/data/daily_summary.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)
🪛 GitHub Actions: CI
src/garth/data/daily_summary.py
[error] 57-57: mypy: Argument 1 to 'camel_to_snake_dict' has incompatible type 'dict[str, Any] | list[dict[str, Any]]'; expected 'dict[str, Any]'.
🔇 Additional comments (2)
src/garth/data/daily_summary.py (2)
1-8: LGTM: Clean imports.All imports are properly organized and used within the file.
11-43: LGTM: Well-structured data model.The DailySummary dataclass is appropriately designed with required identity fields (
user_profile_id,calendar_date) and comprehensive optional health metrics. Field types are correct.
| @classmethod | ||
| def get( | ||
| cls, day: date | str, *, client: http.Client | None = None | ||
| ) -> Self | None: | ||
| client = client or http.client | ||
| path = f"/usersummary-service/usersummary/daily/?calendarDate={day}" | ||
| daily_summary = client.connectapi(path) | ||
|
|
||
| # pragma: no cover | ||
| if not daily_summary: | ||
| return None | ||
|
|
||
| daily_summary = camel_to_snake_dict(daily_summary) | ||
| assert isinstance(daily_summary, dict) | ||
| return cls(**daily_summary) |
There was a problem hiding this comment.
Fix type handling to resolve mypy error.
The mypy pipeline failure on line 57 is valid: client.connectapi() returns dict[str, Any] | list[dict[str, Any]] | None, but camel_to_snake_dict expects only dict[str, Any]. The isinstance check on line 58 comes too late—it needs to occur before calling camel_to_snake_dict to satisfy mypy and prevent passing a list.
Additionally, consider replacing the falsy check with an explicit is None check for clarity.
🔧 Proposed fix for type handling
@classmethod
def get(
cls, day: date | str, *, client: http.Client | None = None
) -> Self | None:
client = client or http.client
path = f"/usersummary-service/usersummary/daily/?calendarDate={day}"
daily_summary = client.connectapi(path)
- # pragma: no cover
- if not daily_summary:
+ if daily_summary is None:
return None
+
+ if not isinstance(daily_summary, dict):
+ raise TypeError(f"Expected dict from API, got {type(daily_summary).__name__}")
daily_summary = camel_to_snake_dict(daily_summary)
- assert isinstance(daily_summary, dict)
return cls(**daily_summary)This fix:
- Explicitly checks for
None(clearer intent) - Validates the type before calling
camel_to_snake_dict(resolves mypy error) - Raises a descriptive error if the API unexpectedly returns a list
- Removes the
# pragma: no coversince the None path should now be testable
🧰 Tools
🪛 GitHub Actions: CI
[error] 57-57: mypy: Argument 1 to 'camel_to_snake_dict' has incompatible type 'dict[str, Any] | list[dict[str, Any]]'; expected 'dict[str, Any]'.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/data/test_daily_summary.py (1)
1-25: LGTM! Tests provide adequate happy-path coverage.The tests follow coding guidelines and established patterns:
- Use VCR for HTTP replay
- No return type hints on test functions (as required)
- Correct import ordering
- max_workers=1 for deterministic execution
The current tests validate basic functionality for both single-date and multi-date retrieval scenarios.
Optional: Consider adding edge case tests
For more comprehensive coverage, you could add tests for:
- Dates with no activity data (to verify None handling)
- API error responses
- Invalid date formats
However, these are nice-to-have improvements and not required for the initial implementation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
tests/data/cassettes/test_daily_summary_get.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_daily_summary_list.yamlis excluded by!tests/**/cassettes/**
📒 Files selected for processing (5)
README.mdsrc/garth/__init__.pysrc/garth/data/__init__.pysrc/garth/data/daily_summary.pytests/data/test_daily_summary.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/garth/data/daily_summary.py
🧰 Additional context used
📓 Path-based instructions (4)
**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.py: Usemake formatto auto-format Python source files using ruff
Usemake lintto lint Python source files (ruff format check, ruff check, mypy)
Files:
src/garth/__init__.pysrc/garth/data/__init__.pytests/data/test_daily_summary.py
**/__init__.py
📄 CodeRabbit inference engine (CLAUDE.md)
Provide the main client instance as
garth.clientfor direct API access
Files:
src/garth/__init__.pysrc/garth/data/__init__.py
tests/**/*.py
📄 CodeRabbit inference engine (CLAUDE.md)
tests/**/*.py: Use pytest with VCR cassettes for HTTP recording/playback in tests
Maintain comprehensive test coverage across all modules with separate test directories mirroring source structure
Files:
tests/data/test_daily_summary.py
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_daily_summary.py
🧠 Learnings (1)
📚 Learning: 2026-01-08T00:25:14.763Z
Learnt from: CR
Repo: matin/garth PR: 0
File: CLAUDE.md:0-0
Timestamp: 2026-01-08T00:25:14.763Z
Learning: Applies to data/**/*.py : Raw data retrieval from Garmin Connect API should be organized in the `data/` module with submodules for specific data types (body_battery/, hrv.py, sleep.py, weight.py)
Applied to files:
src/garth/__init__.pysrc/garth/data/__init__.py
🧬 Code graph analysis (3)
src/garth/__init__.py (1)
src/garth/data/daily_summary.py (1)
DailySummary(12-61)
src/garth/data/__init__.py (1)
src/garth/data/daily_summary.py (1)
DailySummary(12-61)
tests/data/test_daily_summary.py (3)
src/garth/data/daily_summary.py (2)
DailySummary(12-61)get(46-61)src/garth/http.py (1)
Client(19-245)tests/conftest.py (2)
authed_client(65-75)client(21-22)
🔇 Additional comments (3)
README.md (1)
908-1027: LGTM! Clear and comprehensive documentation.The Daily Summary documentation follows the established pattern and provides clear examples for both single-date and date-range retrieval. The sample data fields are well-illustrated and match the API surface.
src/garth/__init__.py (1)
4-4: LGTM! Correct public API exposure.The DailySummary import and export follow the established pattern and maintain alphabetical ordering consistently with other data classes in the public API.
Also applies to: 37-37
src/garth/data/__init__.py (1)
6-6: LGTM! Proper module exposure.The DailySummary export and import follow the established pattern and maintain alphabetical ordering with other data classes in the module.
Also applies to: 22-22
2 participants
Hi, matin,
I have added an option to pull daily summary for a user as I believe it is nice to have simple overview available directly.
I wasn't sure if it belongs in data or stats, but given the format of the request, I have chosen the data directory as the best possible place.
Please, let me know if there is anything else that needs to be done.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.