Add an option to list daily summary #132
Conversation
|
""" WalkthroughA new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant DailySummary
participant http.Client
participant API
User->>DailySummary: DailySummary.get(day, client)
DailySummary->>http.Client: get("/wellness/dailySummary", params=day)
http.Client->>API: HTTP GET /wellness/dailySummary?date=day
API-->>http.Client: Response (daily summary data)
http.Client-->>DailySummary: Response data
DailySummary->>DailySummary: Convert keys, validate, instantiate object
DailySummary-->>User: DailySummary instance or None
Suggested reviewers
✨ 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
|
![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.
| @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) | ||
| 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.
🛠️ Refactor suggestion
Improve error handling and input validation.
The method has several areas for improvement:
- Input validation: No validation for string date format
- Error handling: API failures aren't handled beyond checking data existence
- Assertion improvement: The assertion lacks context for debugging
Consider this improved implementation:
@classmethod
def get(
cls, day: date | str, *, client: http.Client | None = None
) -> Self | None:
+ # Validate string date format if needed
+ if isinstance(day, str):
+ try:
+ # Attempt to parse to validate format
+ from datetime import datetime
+ datetime.strptime(str(day), '%Y-%m-%d')
+ except ValueError as e:
+ raise ValueError(f"Invalid date format '{day}'. Expected YYYY-MM-DD") from e
+
client = client or http.client
path = f"/usersummary-service/usersummary/daily/?calendarDate={day}"
- daily_summary = client.connectapi(path)
+ try:
+ daily_summary = client.connectapi(path)
+ except Exception as e:
+ # Log the error or handle appropriately based on your error handling strategy
+ raise RuntimeError(f"Failed to fetch daily summary for {day}") from e
+
if not daily_summary:
return None
daily_summary = camel_to_snake_dict(daily_summary)
- assert isinstance(daily_summary, dict)
+ if not isinstance(daily_summary, dict):
+ raise ValueError(f"Expected dict from API, got {type(daily_summary)}")
return cls(**daily_summary)📝 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 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) | |
| if not daily_summary: | |
| return None | |
| daily_summary = camel_to_snake_dict(daily_summary) | |
| assert isinstance(daily_summary, dict) | |
| return cls(**daily_summary) | |
| @classmethod | |
| def get( | |
| cls, day: date | str, *, client: http.Client | None = None | |
| ) -> Self | None: | |
| # Validate string date format if needed | |
| if isinstance(day, str): | |
| try: | |
| # Attempt to parse to validate format | |
| from datetime import datetime | |
| datetime.strptime(str(day), '%Y-%m-%d') | |
| except ValueError as e: | |
| raise ValueError(f"Invalid date format '{day}'. Expected YYYY-MM-DD") from e | |
| client = client or http.client | |
| path = f"/usersummary-service/usersummary/daily/?calendarDate={day}" | |
| try: | |
| daily_summary = client.connectapi(path) | |
| except Exception as e: | |
| # Log the error or handle appropriately based on your error handling strategy | |
| raise RuntimeError(f"Failed to fetch daily summary for {day}") from e | |
| if not daily_summary: | |
| return None | |
| daily_summary = camel_to_snake_dict(daily_summary) | |
| if not isinstance(daily_summary, dict): | |
| raise ValueError(f"Expected dict from API, got {type(daily_summary)}") | |
| return cls(**daily_summary) |
🤖 Prompt for AI Agents
In src/garth/data/daily_summary.py lines 41 to 52, improve the get method by
adding validation to ensure the day parameter, if a string, matches the expected
date format before using it. Enhance error handling by catching exceptions from
the client.connectapi call and handling them gracefully instead of only checking
for falsy data. Replace the bare assertion with a conditional check that raises
a descriptive error if daily_summary is not a dictionary, providing context for
debugging. These changes will make the method more robust and easier to
troubleshoot.
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
from
3d14c9c to
65d9bd4
Compare
vactomas
force-pushed
the
feature/daily-summary
branch
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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #132 +/- ##
===========================================
- Coverage 100.00% 99.94% -0.06%
===========================================
Files 45 47 +2
Lines 1898 1965 +67
===========================================
+ Hits 1898 1964 +66
- Misses 0 1 +1
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? |
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
Summary by CodeRabbit