feat: Activity dataclass#87
Conversation
WalkthroughAdds a new Activity data module with Pydantic dataclasses and class methods to fetch/list Garmin activities, introduces DTO-suffix removal utilities, and exposes Activity on the package public API; includes tests for activity endpoints and the new utilities. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Activity as Activity.get()
participant HTTP as http.client
participant API as /activity-service/activity/{id}
Caller->>Activity: get(activity_id)
Activity->>HTTP: HTTP GET /activity-service/activity/{id}
HTTP->>API: request
API-->>HTTP: JSON response (camelCase, DTO keys)
HTTP-->>Activity: response dict
Activity->>Activity: camel_to_snake_dict()
Activity->>Activity: remove_dto_suffix_from_dict()
Activity->>Activity: construct Activity instance
Activity-->>Caller: Activity
sequenceDiagram
participant Caller as Caller
participant Activity as Activity.list()
participant HTTP as http.client
participant API as /activitylist-service/activities/search/activities
Caller->>Activity: list(limit, start)
Activity->>HTTP: HTTP POST/GET with pagination params
HTTP->>API: request
API-->>HTTP: JSON list response (camelCase, DTO keys)
HTTP-->>Activity: response list
loop per item
Activity->>Activity: camel_to_snake_dict()
Activity->>Activity: remove_dto_suffix_from_dict()
Activity->>Activity: construct Activity instance
end
Activity-->>Caller: list[Activity]
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 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: 6
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
Makefile(1 hunks)garth/data/__init__.py(1 hunks)garth/data/activity.py(1 hunks)garth/utils.py(1 hunks)pyproject.toml(1 hunks)
🔇 Additional comments (7)
garth/data/__init__.py (2)
1-1: Add 'Activity' to the module's public API
This change looks good and aligns with making the Activity class a first-class exported entity.
3-3: Importing Activity
No apparent issues, and consistency with the updated __all__ is maintained.
garth/utils.py (1)
36-43: Check for consistent suffix handling
The function correctly handles both "_dto" and "DTO" suffixes. The logic appears straightforward, and no issues are found with the slicing.
garth/data/activity.py (4)
9-13: Ensure consistent type usage
ActivityType is a minimal, frozen dataclass that correctly represents activity metadata. If you anticipate changes (e.g., additional fields), ensure future expansions remain consistent with Activity.
72-80: Check optional fields
The optional cadence/steps attributes help track running metrics. Ensure that downstream usage handles None values gracefully to avoid potential TypeErrors.
82-91: Revisit offset approach
The _get_localized_datetime method calculates the local offset and applies it to the GMT timestamp. Watch out for edge cases like daylight saving time transitions or historical offsets outside standard rules.
92-97: Exposing activity_start
This property elegantly exposes a localized start time. It provides clarity and ensures consistent usage with _get_localized_datetime.
| uv run ruff format --check $(sources) | ||
| uv run ruff check $(sources) | ||
| uv run mypy $(sources) | ||
| uv run --group linting mypy $(sources) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure consistent usage of dependency groups
The lint target uses the new --group linting flag for mypy but not for ruff commands. This inconsistency could lead to confusion about how dependencies are managed.
Consider updating all linting commands to use the same approach:
.PHONY: lint ## Lint python source files
lint: .uv
- uv run ruff format --check $(sources)
- uv run ruff check $(sources)
- uv run --group linting mypy $(sources)
+ uv run --group linting ruff format --check $(sources)
+ uv run --group linting ruff check $(sources)
+ uv run --group linting mypy $(sources)Also, the install target needs to be updated to match the new dependency management approach:
.PHONY: install
install: .uv .pre-commit
- uv pip install -e ".[dev,linting,testing]"
+ uv pip install -e ".[dev,testing]" --group lintingCommittable suggestion skipped: line range outside the PR's diff.
|
@juvilius I apologize for not getting around to reviewing earlier. do you want to rebase, add tests and submit the changes? adding this endpoint makes sense, especially to make available in |
|
Hi, @juvilius, |
|
Hi @matin, sorry for the holdup. Let me explain. Long story short, I don't know how to easily collect every possible field for all types of |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #87 +/- ##
==========================================
Coverage 100.00% 100.00%
==========================================
Files 51 53 +2
Lines 2113 2309 +196
==========================================
+ Hits 2113 2309 +196
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:
|
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In @src/garth/data/activity.py:
- Around line 129-146: Duplicate metric fields (distance, duration,
elapsed_duration, moving_duration, elevation_gain, elevation_loss,
average_speed, max_speed, calories, average_hr, max_hr, owner_id,
owner_display_name, owner_full_name, steps,
average_running_cadence_in_steps_per_minute,
max_running_cadence_in_steps_per_minute) in Activity conflict with
ActivitySummary and should be removed or relocated to avoid ambiguity. Fix by
splitting list-vs-detail representations: create a new dataclass
ActivityListItem (or similar) that contains the list-specific fields currently
declared on Activity, keep Activity as the detailed model (removing the
duplicated metric fields) and add an optional summary: ActivitySummary | None to
Activity; alternatively add a single nested field like list_metadata:
ActivityListMetadata on Activity and move those fields there. Update any code
constructing Activity (e.g., where Activity is built from list() responses) to
use ActivityListItem or populate list_metadata instead, and update docstrings
for Activity/ActivityListItem to state which endpoint populates which fields.
- Around line 148-173: The get classmethod currently uses assert statements to
validate the API response; replace these asserts with explicit exception
handling: after calling client.connectapi(path) check if data is falsy and raise
a clear exception (e.g., RuntimeError or a custom ActivityFetchError) that
includes the request path and any client error info, and instead of asserting
the type, raise a TypeError or ValueError with a message like "Expected dict
from {path}, got {type(data).__name__}"; keep the rest of the flow
(camel_to_snake_dict, remove_dto_from_dict, return cls(**data)) unchanged but
ensure exceptions include the activity_id and path for easier debugging.
- Around line 175-203: The list classmethod should validate parameters and
replace the runtime assert with proper exceptions: check that limit and start
are integers and non-negative (raise TypeError for wrong types and ValueError
for negative values) before calling http.client.connectapi, then call client =
client or http.client and path =
"/activitylist-service/activities/search/activities" as before; after calling
client.connectapi, if the returned data is not a list raise a RuntimeError (or a
custom exception) including the path and the actual type received instead of
using assert, and keep the loop that transforms items with camel_to_snake_dict
and constructs cls(**item).
- Line 10: Add unit tests in tests/test_utils.py for both camel_to_snake_dict
and remove_dto_from_dict: create test cases with nested dictionaries, lists of
dictionaries, empty structures, non-dict scalars, and realistic Garmin API-like
payloads to verify recursive behavior; for camel_to_snake_dict assert keys are
converted at all nesting levels and that existing snake_case keys and non-string
values are preserved; for remove_dto_from_dict assert that DTO wrapper keys
(e.g., keys named "dto" or with "Dto" suffix) are removed or unwrapped
recursively from nested dicts and lists and that original payload values are
retained after removal. Use the imported functions camel_to_snake_dict and
remove_dto_from_dict in these tests and include multiple assertions covering
edge cases (deep nesting, lists containing mixed types) to ensure full coverage.
- Around line 1-14: The project currently imports ConfigDict (a Pydantic 2.0+
API) in activity.py but the pyproject ranges pydantic>=1.10.12,<3.0.0 which
allows Pydantic 1.x; update the dependency constraint to pydantic>=2.0.0,<3.0.0
in pyproject.toml so the installed Pydantic supports ConfigDict usage (no code
changes needed in activity.py; keep the ConfigDict import and pydantic_config
as-is).
🧹 Nitpick comments (4)
src/garth/utils.py (1)
36-62: LGTM! Utility functions correctly handle DTO suffix removal.The implementation correctly strips DTO suffixes from keys and recursively processes nested structures, mirroring the pattern established by
camel_to_snake_dict.💡 Optional: Simplify remove_dto logic
The two branches in
remove_dtoperform the same operation. Consider this more concise version:def remove_dto(key: str) -> str: - if key.endswith("_dto"): - return key[: -len("_dto")] - elif key.endswith("DTO"): - return key[: -len("DTO")] - else: - return key + for suffix in ("_dto", "DTO"): + if key.endswith(suffix): + return key[:-len(suffix)] + return keyThis reduces duplication while maintaining the same behavior.
tests/data/test_activity.py (1)
18-32: LGTM! Thorough validation of get endpoint.The test correctly validates the detailed activity retrieval, including summary data. The pattern of fetching an ID from the list first is appropriate for API integration tests with VCR cassettes.
💡 Optional: Improve assertion clarity on Line 31
For consistency with the
distance is not Nonecheck, consider:- assert activity.summary.distance is not None or activity.summary.duration + assert activity.summary.distance is not None or activity.summary.duration is not NoneThis makes the intent clearer (checking for presence of either metric) rather than relying on truthiness of the duration value.
src/garth/data/activity.py (2)
42-98: Consider documenting units for metric fields.Many fields in
ActivitySummaryrepresent measurements (distance, duration, speed, elevation, etc.) but don't document their units. Users may need to know:
- Is
distancein meters, kilometers, or miles?- Is
durationin seconds or milliseconds?- Is
elevation_gainin meters or feet?- What are the units for
average_speed?Adding this information to field docstrings or the class docstring would improve usability.
📝 Example documentation approach
@dataclass(config=pydantic_config) class ActivitySummary: """Summary metrics for an activity. Most fields are optional since different activity types return different metrics. For example, running activities have cadence data while swimming activities have stroke data. Units: - distance: meters - duration/elapsed_duration/moving_duration: seconds - speed: meters per second - elevation: meters - temperature: Celsius - power: watts - heart rate: beats per minute """
42-43: Clarify timezone handling for datetime fields.The
start_time_localandstart_time_gmtfields (and others) usedatetimebut don't specify whether these are timezone-aware or naive datetime objects. Document the expected timezone behavior to help users handle these correctly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
tests/data/cassettes/test_activity_get.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_activity_list.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_activity_list_pagination.yamlis excluded by!tests/**/cassettes/**
📒 Files selected for processing (6)
Makefilesrc/garth/__init__.pysrc/garth/data/__init__.pysrc/garth/data/activity.pysrc/garth/utils.pytests/data/test_activity.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/utils.pysrc/garth/data/__init__.pytests/data/test_activity.pysrc/garth/data/activity.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_activity.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_activity.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 **/*.py : Use `make lint` to lint Python source files (ruff format check, ruff check, mypy)
Applied to files:
Makefile
🧬 Code graph analysis (3)
src/garth/__init__.py (1)
src/garth/data/activity.py (1)
Activity(102-203)
src/garth/data/__init__.py (1)
src/garth/data/activity.py (1)
Activity(102-203)
tests/data/test_activity.py (3)
src/garth/data/activity.py (2)
Activity(102-203)get(149-173)src/garth/http.py (1)
Client(19-245)tests/conftest.py (3)
vcr(79-82)authed_client(65-75)client(21-22)
🔇 Additional comments (8)
src/garth/data/__init__.py (1)
1-29: LGTM! Clean public API exposure.The Activity export follows the established pattern for other data classes in this module. The addition to
__all__and the import statement are correctly placed in alphabetical order.Makefile (1)
34-34: LGTM! Aligns with dependency group strategy.Moving mypy to the
lintingdependency group is consistent with the PR's goal of using dependency groups instead of optional dependencies. This change properly isolates linting tools.Based on learnings, this maintains the expected
make lintbehavior while improving dependency organization.src/garth/__init__.py (1)
1-67: LGTM! Consistent top-level API exposure.Activity is properly exposed at the package level, following the established pattern for other data classes. The alphabetical ordering in both the import statement and
__all__is maintained.tests/data/test_activity.py (2)
7-16: LGTM! Comprehensive validation of list endpoint.The test properly validates the list endpoint with appropriate assertions for activity fields and nested type information. The use of
@pytest.mark.vcrfollows the project's testing guidelines.
34-46: LGTM! Excellent pagination validation.The test thoroughly validates pagination behavior, including the important check that pages contain distinct activities using
isdisjoint(). This ensures the pagination parameters work correctly.src/garth/data/activity.py (3)
16-30: LGTM!The
ActivityTypeandEventTypedataclasses are well-structured with appropriate use of optional fields.
33-98: LGTM! Well-documented design choice.The
ActivitySummarydataclass appropriately handles the varying nature of activity data by making all metric fields optional. The docstring clearly explains this design decision, and the grouping of swimming-specific fields aids readability.
1-203: Ensure Pydantic mypy plugin is configured inpyproject.tomlormypy.ini.The code uses Pydantic dataclasses and
Selffromtyping_extensions. For proper type checking:
- Add
plugins = ["pydantic.mypy"]to[tool.mypy]inpyproject.toml(use"pydantic.v1.mypy"for Pydantic v1)- Verify mypy version is 1.19+ (required for
Selfsupport withtyping_extensions)- Run type checking via
make lintto confirm all type annotations resolve without errors
| from __future__ import annotations | ||
|
|
||
| from datetime import datetime | ||
|
|
||
| from pydantic import ConfigDict | ||
| from pydantic.dataclasses import dataclass | ||
| from typing_extensions import Self | ||
|
|
||
| from .. import http | ||
| from ..utils import camel_to_snake_dict, remove_dto_from_dict | ||
|
|
||
|
|
||
| pydantic_config = ConfigDict(extra="ignore") | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd -t f -E node_modules -E .git 'pyproject\.toml|setup\.py|requirements.*\.txt' | head -20Repository: matin/garth
Length of output: 69
🏁 Script executed:
cat pyproject.tomlRepository: matin/garth
Length of output: 2105
🏁 Script executed:
# Check for other uses of ConfigDict in the codebase
rg "ConfigDict" --type py
# Check for other imports of typing_extensions
rg "from typing_extensions import|import typing_extensions" --type pyRepository: matin/garth
Length of output: 1091
🏁 Script executed:
# Check if typing_extensions is a transitive dependency by looking at Pydantic's own dependencies
# or if it's mentioned anywhere in lock files
fd -t f 'poetry.lock|requirements.lock|uv.lock|pip-requirements.txt' --exec cat {} \;
# Also check if there's any documentation about dependencies
cat README.md 2>/dev/null | head -50Repository: matin/garth
Length of output: 50368
🏁 Script executed:
# Search for typing_extensions and pydantic in the lock file
rg "name = \"typing.extensions|name = \"pydantic" pyproject.tomlRepository: matin/garth
Length of output: 37
🏁 Script executed:
# Search for typing_extensions in the lock file
rg "^\[\[package\]\]|^name = \"typing" pyproject.toml | head -20
# Try searching in lock file
rg '^\[\[package\]\]' pyproject.toml | head -10
# Actually, let's check if there's a uv.lock file or poetry.lock
ls -la | grep -i lockRepository: matin/garth
Length of output: 113
🏁 Script executed:
# Search for typing_extensions in uv.lock
rg 'name = "typing.extensions"' uv.lock -A 5
# Search for pydantic in uv.lock
rg 'name = "pydantic"' uv.lock -A 10Repository: matin/garth
Length of output: 7416
Fix Pydantic version constraint to match ConfigDict usage.
The code imports ConfigDict from Pydantic on line 5, which is a Pydantic 2.0+ feature that does not exist in Pydantic 1.10.12. The pyproject.toml dependency constraint pydantic>=1.10.12,<3.0.0 allows Pydantic 1.10.12, making installation incompatible. Update the constraint to pydantic>=2.0.0,<3.0.0 to enforce Pydantic 2.0+.
Note: typing_extensions is correctly available as a transitive dependency from Pydantic and does not need to be added separately.
🤖 Prompt for AI Agents
In @src/garth/data/activity.py around lines 1 - 14, The project currently
imports ConfigDict (a Pydantic 2.0+ API) in activity.py but the pyproject ranges
pydantic>=1.10.12,<3.0.0 which allows Pydantic 1.x; update the dependency
constraint to pydantic>=2.0.0,<3.0.0 in pyproject.toml so the installed Pydantic
supports ConfigDict usage (no code changes needed in activity.py; keep the
ConfigDict import and pydantic_config as-is).
| from typing_extensions import Self | ||
|
|
||
| from .. import http | ||
| from ..utils import camel_to_snake_dict, remove_dto_from_dict |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if utils.py exists and contains the required functions
echo "=== Checking for function implementations ==="
rg -A 15 "def (camel_to_snake_dict|remove_dto_from_dict)" src/garth/utils.py
echo -e "\n=== Checking for existing tests ==="
rg -l "camel_to_snake|remove_dto" --type=py | head -20
echo -e "\n=== Checking imports in activity.py ==="
head -20 src/garth/data/activity.py
echo -e "\n=== Checking usage context in activity.py ==="
rg -B 2 -A 5 "(camel_to_snake_dict|remove_dto_from_dict)" src/garth/data/activity.pyRepository: matin/garth
Length of output: 2847
🏁 Script executed:
#!/bin/bash
echo "=== Checking test coverage for utility functions ==="
cat tests/test_utils.py
echo -e "\n=== Verify helper functions that these depend on ==="
rg -A 5 "def camel_to_snake|def remove_dto" src/garth/utils.py
echo -e "\n=== Check lines 171-172 and 201 in activity.py ==="
sed -n '165,210p' src/garth/data/activity.pyRepository: matin/garth
Length of output: 4082
Add missing test for remove_dto_from_dict and expand edge case coverage.
The utility functions camel_to_snake_dict and remove_dto_from_dict are properly implemented with recursive handling for nested dictionaries and lists. However, remove_dto_from_dict lacks test coverage entirely—it's imported in tests/test_utils.py but has no test function. Additionally, test_camel_to_snake_dict() only tests basic key conversion; neither function is tested against edge cases like nested dictionaries, lists of dictionaries, or the actual API response structures from Garmin.
Add comprehensive tests for both functions, including nested and complex structures that match real API responses.
🤖 Prompt for AI Agents
In @src/garth/data/activity.py at line 10, Add unit tests in tests/test_utils.py
for both camel_to_snake_dict and remove_dto_from_dict: create test cases with
nested dictionaries, lists of dictionaries, empty structures, non-dict scalars,
and realistic Garmin API-like payloads to verify recursive behavior; for
camel_to_snake_dict assert keys are converted at all nesting levels and that
existing snake_case keys and non-string values are preserved; for
remove_dto_from_dict assert that DTO wrapper keys (e.g., keys named "dto" or
with "Dto" suffix) are removed or unwrapped recursively from nested dicts and
lists and that original payload values are retained after removal. Use the
imported functions camel_to_snake_dict and remove_dto_from_dict in these tests
and include multiple assertions covering edge cases (deep nesting, lists
containing mixed types) to ensure full coverage.
| # Fields from list endpoint (not present in detail endpoint) | ||
| distance: float | None = None | ||
| duration: float | None = None | ||
| elapsed_duration: float | None = None | ||
| moving_duration: float | None = None | ||
| elevation_gain: float | None = None | ||
| elevation_loss: float | None = None | ||
| average_speed: float | None = None | ||
| max_speed: float | None = None | ||
| calories: float | None = None | ||
| average_hr: float | None = None | ||
| max_hr: float | None = None | ||
| owner_id: int | None = None | ||
| owner_display_name: str | None = None | ||
| owner_full_name: str | None = None | ||
| steps: int | None = None | ||
| average_running_cadence_in_steps_per_minute: float | None = None | ||
| max_running_cadence_in_steps_per_minute: float | None = None |
There was a problem hiding this comment.
Address field duplication between Activity and ActivitySummary.
The duplicate fields (lines 130-146) that overlap with ActivitySummary create ambiguity about which fields should be accessed. Users may not know whether to read activity.distance or activity.summary.distance, and the behavior differs depending on whether the Activity was retrieved via get() or list().
Consider one of these approaches:
- Create a separate
ActivityListItemdataclass for the list endpoint response - Document clearly in the docstring which fields are populated by which endpoint
- Move list-specific fields to a nested
list_metadataor similar field
💡 Suggested refactoring approach
Option 1: Separate dataclasses (recommended)
@dataclass(config=pydantic_config)
class ActivityListItem:
"""Simplified activity data from list endpoint."""
activity_id: int
activity_name: str
activity_type: ActivityType
start_time_local: datetime | None = None
distance: float | None = None
# ... other list-specific fields
@dataclass(config=pydantic_config)
class Activity:
"""Detailed activity data from get endpoint."""
activity_id: int
activity_name: str
activity_type: ActivityType
summary: ActivitySummary | None = None
# ... no duplicate fieldsOption 2: Enhanced documentation
Add to the Activity docstring:
"""
Note on field availability:
- When retrieved via get(): summary contains metrics, top-level metric fields are None
- When retrieved via list(): top-level metric fields are populated, summary is None
"""🤖 Prompt for AI Agents
In @src/garth/data/activity.py around lines 129 - 146, Duplicate metric fields
(distance, duration, elapsed_duration, moving_duration, elevation_gain,
elevation_loss, average_speed, max_speed, calories, average_hr, max_hr,
owner_id, owner_display_name, owner_full_name, steps,
average_running_cadence_in_steps_per_minute,
max_running_cadence_in_steps_per_minute) in Activity conflict with
ActivitySummary and should be removed or relocated to avoid ambiguity. Fix by
splitting list-vs-detail representations: create a new dataclass
ActivityListItem (or similar) that contains the list-specific fields currently
declared on Activity, keep Activity as the detailed model (removing the
duplicated metric fields) and add an optional summary: ActivitySummary | None to
Activity; alternatively add a single nested field like list_metadata:
ActivityListMetadata on Activity and move those fields there. Update any code
constructing Activity (e.g., where Activity is built from list() responses) to
use ActivityListItem or populate list_metadata instead, and update docstrings
for Activity/ActivityListItem to state which endpoint populates which fields.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In @src/garth/data/activity.py:
- Around line 196-198: Replace the runtime-optimizable assert with an explicit
type check: instead of using assert isinstance(data, list), add an if not
isinstance(data, list): raise TypeError(f"Expected list from {path}, got
{type(data).__name__}") so invalid input cannot slip through when Python is run
with -O; keep the same message and reference to data and path to preserve
context.
- Around line 199-202: The list method in src/garth/data/activity.py only calls
camel_to_snake_dict but must also strip DTO-suffixed keys like the get method
does; update the loop that builds activities (where camel_to_snake_dict(item)
and activities.append(cls(**item)) are used) to call remove_dto_from_dict after
converting keys (e.g., item = camel_to_snake_dict(item); item =
remove_dto_from_dict(item)) so each dict passed to cls(**item) is normalized
consistently with the get method.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
tests/data/cassettes/test_activity_get.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_activity_list.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_activity_list_pagination.yamlis excluded by!tests/**/cassettes/**
📒 Files selected for processing (5)
src/garth/__init__.pysrc/garth/data/__init__.pysrc/garth/data/activity.pysrc/garth/utils.pytests/data/test_activity.py
🚧 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 (3)
**/*.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/utils.pytests/data/test_activity.pysrc/garth/data/activity.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_activity.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_activity.py
🧠 Learnings (2)
📚 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/data/activity.py
📚 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 tests/**/*.py : Maintain comprehensive test coverage across all modules with separate test directories mirroring source structure
Applied to files:
src/garth/data/activity.py
🧬 Code graph analysis (2)
tests/data/test_activity.py (1)
src/garth/data/activity.py (2)
Activity(102-203)get(149-173)
src/garth/data/activity.py (3)
src/garth/utils.py (2)
camel_to_snake_dict(17-33)remove_dto_from_dict(45-62)tests/conftest.py (1)
client(21-22)src/garth/http.py (2)
Client(19-245)connectapi(186-192)
🔇 Additional comments (9)
src/garth/utils.py (2)
36-42: LGTM!The function correctly handles both
_dtoandDTOsuffix removal with clear logic.
45-62: LGTM!The recursive implementation correctly handles nested dictionaries and lists, and follows the same pattern as the existing
camel_to_snake_dictfunction for consistency.tests/data/test_activity.py (3)
7-15: LGTM!The test appropriately validates that the list endpoint returns activities with the expected basic fields.
18-31: LGTM!The test correctly validates the get endpoint by first obtaining an activity ID from the list, then fetching full details. The assertion on Line 31 appropriately checks that at least one summary metric is present.
34-46: LGTM!The pagination test appropriately validates that different pages return distinct activities and that the pagination parameters work as expected.
src/garth/data/activity.py (4)
1-13: LGTM!The use of
extra="ignore"in the Pydantic config is appropriate for handling varying activity fields across different activity types and devices, which addresses the author's stated concern about field variation.
16-23: LGTM!The
ActivityTypedataclass has appropriate fields and type annotations.
26-30: LGTM!The
EventTypedataclass has appropriate fields and type annotations.
33-98: LGTM!The
ActivitySummarydataclass comprehensively covers various activity metrics with appropriate optional fields. The docstring clearly explains why most fields are optional, which aligns well with the author's stated approach to handling field variation.
| assert isinstance(data, list), ( | ||
| f"Expected list from {path}, got {type(data).__name__}" | ||
| ) |
There was a problem hiding this comment.
Replace assertion with proper exception.
Lines 196-198 use an assert statement for validation. Assertions can be disabled with Python's -O optimization flag, which would allow invalid data types to pass through unchecked.
🔒 Proposed fix using proper exception
path = "/activitylist-service/activities/search/activities"
data = client.connectapi(path, params={"limit": limit, "start": start})
- assert isinstance(data, list), (
- f"Expected list from {path}, got {type(data).__name__}"
- )
+ if not isinstance(data, list):
+ raise TypeError(
+ f"Expected list from {path}, got {type(data).__name__}"
+ )
activities = []📝 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.
| assert isinstance(data, list), ( | |
| f"Expected list from {path}, got {type(data).__name__}" | |
| ) | |
| path = "/activitylist-service/activities/search/activities" | |
| data = client.connectapi(path, params={"limit": limit, "start": start}) | |
| if not isinstance(data, list): | |
| raise TypeError( | |
| f"Expected list from {path}, got {type(data).__name__}" | |
| ) | |
| activities = [] |
🤖 Prompt for AI Agents
In @src/garth/data/activity.py around lines 196 - 198, Replace the
runtime-optimizable assert with an explicit type check: instead of using assert
isinstance(data, list), add an if not isinstance(data, list): raise
TypeError(f"Expected list from {path}, got {type(data).__name__}") so invalid
input cannot slip through when Python is run with -O; keep the same message and
reference to data and path to preserve context.
and ci: dependency group instead of optional dependency for linting
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/garth/data/activity.py:
- Around line 171-199: In Activity.classmethod list, replace the runtime
assertion that checks the API response with explicit error handling: after
calling client.connectapi(path, params=...), verify if data is a list and if not
raise a clear exception (e.g., TypeError or RuntimeError) that includes the path
and the actual type received; mirror the approach used in get() so callers can
catch the error, and keep the subsequent processing (camel_to_snake_dict and
cls(**item)) unchanged while still using client or http.client as before.
- Around line 144-169: Replace the runtime assertions in Activity.get with
explicit exception raising: after calling client.connectapi(path), if data is
falsy raise a RuntimeError (or custom exception) with a clear message including
the path; if data is not a dict raise a TypeError (or ValueError) that states
the expected dict and the actual type (use type(data).__name__); keep using
camel_to_snake_dict, remove_dto_suffix_from_dict, and cls(**data) after these
checks so failures surface reliably even when Python optimizations are enabled.
🧹 Nitpick comments (3)
src/garth/utils.py (1)
36-40: Minor docstring inconsistency.The docstring mentions "Remove _dto or DTO suffix" but the implementation only checks for
_dto(lowercase). Consider either updating the docstring to match the implementation or extending the function to handle both cases if needed.📝 Option 1: Update docstring to match implementation
- """Remove _dto or DTO suffix from a key.""" + """Remove _dto suffix from a key."""📝 Option 2: Handle both cases (if needed by the API)
def remove_dto_suffix(key: str) -> str: - """Remove _dto or DTO suffix from a key.""" - if key.endswith("_dto"): - return key[:-4] + """Remove _dto or DTO suffix from a key.""" + if key.endswith("_dto") or key.endswith("DTO"): + return key[:-4] if key.endswith("_dto") else key[:-3] return keysrc/garth/data/activity.py (2)
97-142: Consider clarifying endpoint differences in the docstring.The Activity class handles two different API response shapes: the detail endpoint (used by
get()) returns data in asummaryobject, while the list endpoint returns similar fields directly on the activity. While the inline comment on line 125 mentions this, the docstring examples don't clarify which endpoint each example uses.📝 Suggested docstring enhancement
"""Garmin Connect activity data. Retrieve individual activities by ID or list recent activities. + + Note: The detail endpoint (get) returns metrics in the summary field, + while the list endpoint returns metrics directly on the activity. Example: >>> activity = Activity.get(12345678901) >>> activity.activity_name 'Morning Run' >>> activity.summary.distance 5000.0 >>> activities = Activity.list(limit=10) + >>> activities[0].distance # List endpoint returns distance directly + 5000.0 >>> len(activities) 10 """
172-177: Consider adding parameter validation.The
limitandstartparameters lack validation. While the API might handle invalid values, adding bounds checking would provide better error messages and prevent unnecessary API calls.🔍 Suggested parameter validation
@classmethod def list( cls, limit: int = 20, start: int = 0, *, client: http.Client | None = None, ) -> list[Self]: """List recent activities with pagination. Args: limit: Maximum number of activities to return (default 20) start: Offset for pagination (default 0) client: Optional HTTP client (uses default if not provided) Returns: List of Activity instances (simplified, without full summary) """ + if limit <= 0: + raise ValueError(f"limit must be positive, got {limit}") + if start < 0: + raise ValueError(f"start must be non-negative, got {start}") client = client or http.client
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
tests/data/cassettes/test_activity_get.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_activity_list.yamlis excluded by!tests/**/cassettes/**tests/data/cassettes/test_activity_list_pagination.yamlis excluded by!tests/**/cassettes/**
📒 Files selected for processing (6)
src/garth/__init__.pysrc/garth/data/__init__.pysrc/garth/data/activity.pysrc/garth/utils.pytests/data/test_activity.pytests/test_utils.py
🚧 Files skipped from review as they are similar to previous changes (3)
- src/garth/init.py
- src/garth/data/init.py
- tests/data/test_activity.py
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/utils.pytests/test_utils.pysrc/garth/data/activity.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/test_utils.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/test_utils.py
🧠 Learnings (3)
📓 Common learnings
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)
📚 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/data/activity.py
📚 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 tests/**/*.py : Maintain comprehensive test coverage across all modules with separate test directories mirroring source structure
Applied to files:
src/garth/data/activity.py
🧬 Code graph analysis (2)
tests/test_utils.py (1)
src/garth/utils.py (2)
remove_dto_suffix(36-40)remove_dto_suffix_from_dict(43-63)
src/garth/data/activity.py (2)
src/garth/utils.py (2)
camel_to_snake_dict(17-33)remove_dto_suffix_from_dict(43-63)src/garth/http.py (2)
Client(19-245)connectapi(186-192)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test 3.12
🔇 Additional comments (5)
tests/test_utils.py (1)
70-132: LGTM! Comprehensive test coverage.The tests thoroughly cover the new DTO suffix removal utilities including edge cases (empty strings, keys without suffix, nested structures, and lists). The test structure follows existing patterns in the file.
src/garth/utils.py (1)
43-63: LGTM! Clean recursive implementation.The function correctly handles nested dictionaries and lists, mirroring the pattern used in
camel_to_snake_dict. The docstring effectively explains the API inconsistency this function addresses.src/garth/data/activity.py (3)
1-19: LGTM! Clean dataclass definition.The imports are appropriate and the
ActivityTypedataclass is well-structured with clear required and optional fields.
22-26: LGTM!Simple, well-defined dataclass.
29-94: LGTM! Comprehensive summary metrics.The extensive optional fields are well-justified by the docstring, which explains that different activity types return different metrics. This design appropriately handles the varying API responses mentioned in the PR description.
| @classmethod | ||
| def get( | ||
| cls, | ||
| activity_id: int, | ||
| *, | ||
| client: http.Client | None = None, | ||
| ) -> Self: | ||
| """Get detailed activity data by activity ID. | ||
|
|
||
| Args: | ||
| activity_id: The Garmin activity ID | ||
| client: Optional HTTP client (uses default if not provided) | ||
|
|
||
| Returns: | ||
| Activity instance with full details including summary metrics | ||
| """ | ||
| client = client or http.client | ||
| path = f"/activity-service/activity/{activity_id}" | ||
| data = client.connectapi(path) | ||
| assert data, f"No data returned from {path}" | ||
| assert isinstance(data, dict), ( | ||
| f"Expected dict from {path}, got {type(data).__name__}" | ||
| ) | ||
| data = camel_to_snake_dict(data) | ||
| data = remove_dto_suffix_from_dict(data) | ||
| return cls(**data) |
There was a problem hiding this comment.
Replace assertions with proper error handling.
Using assert for runtime validation is problematic because assertions can be disabled with Python's -O optimization flag, which would cause the function to fail silently or with unhelpful errors in production.
🛡️ Proposed fix: Use explicit exception raising
client = client or http.client
path = f"/activity-service/activity/{activity_id}"
data = client.connectapi(path)
- assert data, f"No data returned from {path}"
- assert isinstance(data, dict), (
- f"Expected dict from {path}, got {type(data).__name__}"
- )
+ if not data:
+ raise ValueError(f"No data returned from {path}")
+ if not isinstance(data, dict):
+ raise TypeError(
+ f"Expected dict from {path}, got {type(data).__name__}"
+ )
data = camel_to_snake_dict(data)
data = remove_dto_suffix_from_dict(data)
return cls(**data)📝 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, | |
| activity_id: int, | |
| *, | |
| client: http.Client | None = None, | |
| ) -> Self: | |
| """Get detailed activity data by activity ID. | |
| Args: | |
| activity_id: The Garmin activity ID | |
| client: Optional HTTP client (uses default if not provided) | |
| Returns: | |
| Activity instance with full details including summary metrics | |
| """ | |
| client = client or http.client | |
| path = f"/activity-service/activity/{activity_id}" | |
| data = client.connectapi(path) | |
| assert data, f"No data returned from {path}" | |
| assert isinstance(data, dict), ( | |
| f"Expected dict from {path}, got {type(data).__name__}" | |
| ) | |
| data = camel_to_snake_dict(data) | |
| data = remove_dto_suffix_from_dict(data) | |
| return cls(**data) | |
| @classmethod | |
| def get( | |
| cls, | |
| activity_id: int, | |
| *, | |
| client: http.Client | None = None, | |
| ) -> Self: | |
| """Get detailed activity data by activity ID. | |
| Args: | |
| activity_id: The Garmin activity ID | |
| client: Optional HTTP client (uses default if not provided) | |
| Returns: | |
| Activity instance with full details including summary metrics | |
| """ | |
| client = client or http.client | |
| path = f"/activity-service/activity/{activity_id}" | |
| data = client.connectapi(path) | |
| if not data: | |
| raise ValueError(f"No data returned from {path}") | |
| if not isinstance(data, dict): | |
| raise TypeError( | |
| f"Expected dict from {path}, got {type(data).__name__}" | |
| ) | |
| data = camel_to_snake_dict(data) | |
| data = remove_dto_suffix_from_dict(data) | |
| return cls(**data) |
🤖 Prompt for AI Agents
In @src/garth/data/activity.py around lines 144 - 169, Replace the runtime
assertions in Activity.get with explicit exception raising: after calling
client.connectapi(path), if data is falsy raise a RuntimeError (or custom
exception) with a clear message including the path; if data is not a dict raise
a TypeError (or ValueError) that states the expected dict and the actual type
(use type(data).__name__); keep using camel_to_snake_dict,
remove_dto_suffix_from_dict, and cls(**data) after these checks so failures
surface reliably even when Python optimizations are enabled.
| @classmethod | ||
| def list( | ||
| cls, | ||
| limit: int = 20, | ||
| start: int = 0, | ||
| *, | ||
| client: http.Client | None = None, | ||
| ) -> list[Self]: | ||
| """List recent activities with pagination. | ||
|
|
||
| Args: | ||
| limit: Maximum number of activities to return (default 20) | ||
| start: Offset for pagination (default 0) | ||
| client: Optional HTTP client (uses default if not provided) | ||
|
|
||
| Returns: | ||
| List of Activity instances (simplified, without full summary) | ||
| """ | ||
| client = client or http.client | ||
| path = "/activitylist-service/activities/search/activities" | ||
| data = client.connectapi(path, params={"limit": limit, "start": start}) | ||
| assert isinstance(data, list), ( | ||
| f"Expected list from {path}, got {type(data).__name__}" | ||
| ) | ||
| activities = [] | ||
| for item in data: | ||
| item = camel_to_snake_dict(item) | ||
| activities.append(cls(**item)) | ||
| return activities |
There was a problem hiding this comment.
Replace assertions with proper error handling.
Similar to the get() method, this uses assert for runtime validation which can be disabled with Python's -O flag.
🛡️ Proposed fix: Use explicit exception raising
client = client or http.client
path = "/activitylist-service/activities/search/activities"
data = client.connectapi(path, params={"limit": limit, "start": start})
- assert isinstance(data, list), (
- f"Expected list from {path}, got {type(data).__name__}"
- )
+ if not isinstance(data, list):
+ raise TypeError(
+ f"Expected list from {path}, got {type(data).__name__}"
+ )
activities = []
for item in data:
item = camel_to_snake_dict(item)
activities.append(cls(**item))
return activities📝 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 list( | |
| cls, | |
| limit: int = 20, | |
| start: int = 0, | |
| *, | |
| client: http.Client | None = None, | |
| ) -> list[Self]: | |
| """List recent activities with pagination. | |
| Args: | |
| limit: Maximum number of activities to return (default 20) | |
| start: Offset for pagination (default 0) | |
| client: Optional HTTP client (uses default if not provided) | |
| Returns: | |
| List of Activity instances (simplified, without full summary) | |
| """ | |
| client = client or http.client | |
| path = "/activitylist-service/activities/search/activities" | |
| data = client.connectapi(path, params={"limit": limit, "start": start}) | |
| assert isinstance(data, list), ( | |
| f"Expected list from {path}, got {type(data).__name__}" | |
| ) | |
| activities = [] | |
| for item in data: | |
| item = camel_to_snake_dict(item) | |
| activities.append(cls(**item)) | |
| return activities | |
| @classmethod | |
| def list( | |
| cls, | |
| limit: int = 20, | |
| start: int = 0, | |
| *, | |
| client: http.Client | None = None, | |
| ) -> list[Self]: | |
| """List recent activities with pagination. | |
| Args: | |
| limit: Maximum number of activities to return (default 20) | |
| start: Offset for pagination (default 0) | |
| client: Optional HTTP client (uses default if not provided) | |
| Returns: | |
| List of Activity instances (simplified, without full summary) | |
| """ | |
| client = client or http.client | |
| path = "/activitylist-service/activities/search/activities" | |
| data = client.connectapi(path, params={"limit": limit, "start": start}) | |
| if not isinstance(data, list): | |
| raise TypeError( | |
| f"Expected list from {path}, got {type(data).__name__}" | |
| ) | |
| activities = [] | |
| for item in data: | |
| item = camel_to_snake_dict(item) | |
| activities.append(cls(**item)) | |
| return activities |
🤖 Prompt for AI Agents
In @src/garth/data/activity.py around lines 171 - 199, In Activity.classmethod
list, replace the runtime assertion that checks the API response with explicit
error handling: after calling client.connectapi(path, params=...), verify if
data is a list and if not raise a clear exception (e.g., TypeError or
RuntimeError) that includes the path and the actual type received; mirror the
approach used in get() so callers can catch the error, and keep the subsequent
processing (camel_to_snake_dict and cls(**item)) unchanged while still using
client or http.client as before.
3 participants
and ci: dependency group instead of optional dependency for linting
I've had some troubles pleasing mypy, so please check if I've made a mistake before considering to merge. :)
Also: your
Dataclass is focused on there being one of "it" per day. I'm not sure how to best reconcile it with activities.Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.