feat: Activity dataclass
#87
Conversation
and ci: dependency group instead of optional dependency for linting
WalkthroughThis pull request introduces enhancements to the project's data handling and utility functions, focusing on activity tracking and dependency management. The changes include adding a new Changes
Sequence DiagramsequenceDiagram
participant Client
participant Activity
participant HTTPClient
Client->>Activity: list() or get(id)
alt list activities
Activity->>HTTPClient: Fetch activities
HTTPClient-->>Activity: Return activity data
Activity->>Activity: Sort activities
else get specific activity
Activity->>HTTPClient: Fetch activity by ID
HTTPClient-->>Activity: Return activity details
Activity->>Activity: Process and convert data
end
Activity-->>Client: Return Activity instance(s)
This sequence diagram illustrates the high-level interaction for retrieving activity data, showing how the Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 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.
| [dependency-groups] | ||
| linting = [ | ||
| "mypy>=1.13.0", | ||
| "ruff>=0.8.2", | ||
| "types-requests>=2.32.0.20241016", | ||
| ] |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider consolidating dependency management approach
The new [dependency-groups] section introduces duplicate definitions for linting dependencies that are already defined in [project.optional-dependencies]. This could lead to:
- Version conflicts if different constraints are specified
- Maintenance overhead from managing dependencies in two places
- Confusion about which dependencies are being used
Consider one of these approaches:
- Remove the duplicate definitions and use only
[dependency-groups]:
-[project.optional-dependencies]
-linting = [
- "ruff",
- "mypy",
- "types-requests",
-]
[dependency-groups]
linting = [
"mypy>=1.13.0",
"ruff>=0.8.2",
"types-requests>=2.32.0.20241016",
]- Or, keep using
[project.optional-dependencies]but with the specific versions:
[project.optional-dependencies]
linting = [
- "ruff",
- "mypy",
- "types-requests",
+ "mypy>=1.13.0",
+ "ruff>=0.8.2",
+ "types-requests>=2.32.0.20241016",
]
-[dependency-groups]
-linting = [
- "mypy>=1.13.0",
- "ruff>=0.8.2",
- "types-requests>=2.32.0.20241016",
-]Committable suggestion skipped: line range outside the PR's diff.
| @@ -30,7 +30,7 @@ format: .uv | |||
| lint: .uv | |||
| 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.
| def remove_dto_from_dict(input_dict: Dict[str, Any]) -> Dict[str, Any]: | ||
| """ | ||
| Removes `DTO` suffix from dictionary keys. Different API endpoints give | ||
| back different key names, e.g. "activityTypeDTO" instead of "activityType". | ||
| """ | ||
| output_dict: Dict[str, Any] = {} | ||
| for k, v in input_dict.items(): | ||
| new_key = remove_dto(k) | ||
| if isinstance(v, dict): | ||
| output_dict[new_key] = remove_dto_from_dict(v) | ||
| elif isinstance(v, list): | ||
| output_dict[new_key] = [ | ||
| remove_dto_from_dict(i) if isinstance(i, dict) else i | ||
| for i in v | ||
| ] | ||
| else: | ||
| output_dict[new_key] = v | ||
| return output_dict | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider edge cases for nested structures
This function recursively removes DTO from keys in nested lists and dictionaries. It seems robust enough, but watch out for cases where internal structures might contain conflicting keys once DTO is removed.
| @dataclass(frozen=True) | ||
| class Summary: | ||
| distance: float | ||
| duration: float | ||
| moving_duration: float | ||
| average_speed: float | ||
| max_speed: float | ||
| calories: float | ||
| average_hr: float | ||
| max_hr: float | ||
| start_time_gmt: datetime | ||
| start_time_local: datetime | ||
| start_latitude: float | ||
| start_longitude: float | ||
| elapsed_duration: float | ||
| elevation_gain: float | ||
| elevation_loss: float | ||
| max_elevation: float | ||
| min_elevation: float | ||
| average_moving_speed: float | ||
| bmr_calories: float | ||
| average_run_cadence: float | ||
| max_run_cadence: float | ||
| average_temperature: float | ||
| max_temperature: float | ||
| min_temperature: float | ||
| average_power: float | ||
| max_power: float | ||
| min_power: float | ||
| normalized_power: float | ||
| total_work: float | ||
| ground_contact_time: float | ||
| stride_length: float | ||
| vertical_oscillation: float | ||
| training_effect: float | ||
| anaerobic_training_effect: float | ||
| aerobic_training_effect_message: str | ||
| anaerobic_training_effect_message: str | ||
| vertical_ratio: float | ||
| end_latitude: float | ||
| end_longitude: float | ||
| max_vertical_speed: float | ||
| water_estimated: float | ||
| training_effect_label: str | ||
| activity_training_load: float | ||
| min_activity_lap_duration: float | ||
| moderate_intensity_minutes: float | ||
| vigorous_intensity_minutes: float | ||
| steps: int | ||
| begin_potential_stamina: float | ||
| end_potential_stamina: float | ||
| min_available_stamina: float | ||
| avg_grade_adjusted_speed: float | ||
| difference_body_battery: float | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Validate wide-range numeric fields
Summary has many numeric fields (distances, durations, speeds, HR, etc.). Consider adding validation where necessary (e.g., non-negative checks) to safeguard against malformed input.
| @classmethod | ||
| def get( | ||
| cls, | ||
| id: int, | ||
| *, | ||
| client: http.Client | None = None, | ||
| ): | ||
| client = client or http.client | ||
| path = f"/activity-service/activity/{id}" | ||
| activity_data = client.connectapi(path) | ||
| assert activity_data | ||
| activity_data = camel_to_snake_dict(activity_data) | ||
| activity_data = remove_dto_from_dict(activity_data) | ||
| assert isinstance(activity_data, dict) | ||
| return cls(**activity_data) | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Assert usage and error handling
Currently, the method asserts that activity_data is not None and is a dictionary. If either assertion fails, a more descriptive exception or graceful fallback might be helpful.
| @classmethod | ||
| def list(cls, *args, **kwargs): | ||
| data = super().list(*args, **kwargs) | ||
| return sorted(data, key=lambda x: x.activity_start) |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Sorting approach
list returns a sorted list by activity_start. This is straightforward, but consider the performance impact if the dataset grows large. Paginating or lazily fetching might become necessary in the future.
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
New Features
Activityclass to data module, enabling retrieval and management of activity informationChores
Refactor