Fixed bug with an error happening on add_weight_in #289
Conversation
|
Warning
|
| Cohort / File(s) | Summary |
|---|---|
Weigh-in JSON handlinggarminconnect/__init__.py |
Added _validate_json_exists(response) to return None on HTTP 204 or parse JSON otherwise. Modified add_weigh_in and add_weigh_in_with_timestamps to use the helper and change return type hints to dict[str, Any] | None. |
Sequence Diagram(s)
sequenceDiagram
autonumber
actor User
participant Garmin as Garmin.add_weigh_in
participant Garth as Garth.post
participant Resp as HTTP Response
participant Helper as _validate_json_exists
User->>Garmin: add_weigh_in(payload)
Garmin->>Garth: POST connectapi/url with JSON
Garth-->>Garmin: Response
Garmin->>Helper: validate(response)
alt 204 No Content
Helper-->>Garmin: None
else JSON body
Helper-->>Garmin: Parsed dict
end
Garmin-->>User: dict or None
note over Garmin,Helper: Flow also used by add_weigh_in_with_timestamps
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~10 minutes
Poem
I hopped to post a weight today,
The server nodded, “No content,” okay!
A gentle check—no JSON there—
Still, scales record my bunny flair.
With None or dict, the code’s just right,
My whiskers twitch in bug-free light. 🐰⚖️
Pre-merge checks and finishing touches
❌ Failed checks (2 warnings)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Out of Scope Changes Check | The pull request not only fixes add_weigh_in JSON parsing but also updates add_weigh_in_with_timestamps and return type hints, which are not mentioned in issue #283 and thus extend beyond the stated objective. | Please scope the changes to only the add_weigh_in method as specified in issue #283 or update the linked issue to include the corresponding add_weigh_in_with_timestamps changes. | |
| Docstring Coverage | Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
✅ Passed checks (3 passed)
| Check name | Status | Explanation |
|---|---|---|
| Description Check | ✅ Passed | Check skipped - CodeRabbit’s high-level summary is enabled. |
| Title Check | ✅ Passed | The title clearly indicates that the pull request fixes a bug in the weight addition method by preventing the JSON parsing error when adding a weight entry. |
| Linked Issues Check | ✅ Passed | The changes introduce a validation method that skips JSON parsing on HTTP 204 responses and update add_weigh_in to use it, directly addressing the JSON parsing error described in issue #283. |
✨ Finishing touches
- 📝 Generate Docstrings
🧪 Generate unit tests
- Create PR with unit tests
- Post copyable unit tests in a comment
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 help to get the list of available commands and usage tips.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
garminconnect/__init__.py (2)
687-715: Update the docstring to document the None return case.The return type now includes
Noneto handle HTTP 204 responses, but the docstring doesn't explain whenNoneis returned versus when a dict is returned. This could help users understand the API behavior.Consider adding a Returns section to the docstring:
def add_weigh_in( self, weight: int | float, unitKey: str = "kg", timestamp: str = "" ) -> dict[str, Any] | None: - """Add a weigh-in (default to kg)""" + """Add a weigh-in (default to kg). + + Returns: + dict[str, Any] | None: Response data from the server, or None if the + server returns HTTP 204 No Content (successful submission with no response body). + """
717-760: Update the docstring to document the None return case.Similar to
add_weigh_in, this method's docstring should document whenNoneis returned.def add_weigh_in_with_timestamps( self, weight: int | float, unitKey: str = "kg", dateTimestamp: str = "", gmtTimestamp: str = "", ) -> dict[str, Any] | None: - """Add a weigh-in with explicit timestamps (default to kg)""" + """Add a weigh-in with explicit timestamps (default to kg). + + Returns: + dict[str, Any] | None: Response data from the server, or None if the + server returns HTTP 204 No Content (successful submission with no response body). + """
🧹 Nitpick comments (1)
garminconnect/__init__.py (1)
93-96: Consider adding error handling for JSON parsing.The helper correctly handles HTTP 204, but
response.json()can raiseJSONDecodeErrorif the response body is empty or invalid for other status codes. Consider whether you should validate the response status or add try-except handling.Additionally, you might want to verify that the response was successful before attempting to parse JSON:
def _validate_json_exists(response: requests.Response) -> dict[str, Any] | None: - if response.status_code == 204: - return None - return response.json() + if response.status_code == 204: + return None + # Raise for other error status codes before attempting JSON parsing + response.raise_for_status() + return response.json()This ensures that HTTP errors (4xx, 5xx) are properly raised before attempting JSON parsing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
garminconnect/__init__.py(4 hunks)
🔇 Additional comments (1)
garminconnect/__init__.py (1)
685-685: Verify other POST endpoints for possible 204 responsesSeveral methods still call
.json()directly on POST responses (e.g. add_body_composition, set_blood_pressure, add_hydration_data, request_reload, upload_workout). Confirm none of these endpoints can return HTTP 204; if they do, wrap their calls with _validate_json_exists to prevent JSON parsing errors.
Added a validation method that only returns a response in json if the response status code isn't 204.
This fixed #283
Summary by CodeRabbit