tests/data/test_body_battery_data.py (2)

86-88: Fix line length to comply with style guidelines.

These lines exceed the 79-character limit. Consider reformatting for better readability.

-            assert daily_data.current_body_battery is not None or daily_data.current_body_battery == 0
-            assert daily_data.max_body_battery is not None or daily_data.max_body_battery == 0
-            assert daily_data.min_body_battery is not None or daily_data.min_body_battery == 0
+            assert (daily_data.current_body_battery is not None or 
+                    daily_data.current_body_battery == 0)
+            assert (daily_data.max_body_battery is not None or 
+                    daily_data.max_body_battery == 0)
+            assert (daily_data.min_body_battery is not None or 
+                    daily_data.min_body_battery == 0)

🧰 Tools

🪛 Ruff (0.11.9)

86-86: Line too long (102 > 79)

(E501)

---

87-87: Line too long (94 > 79)

(E501)

---

88-88: Line too long (94 > 79)

(E501)

---

108-110: Minor formatting improvement needed.

Lines exceed the 79-character limit.

-    daily_data_list = DailyBodyBatteryStress.list(end, days, client=authed_client)
+    daily_data_list = DailyBodyBatteryStress.list(
+        end, days, client=authed_client
+    )

-    assert len(daily_data_list) <= days  # May be less if some days have no data
+    # May be less if some days have no data
+    assert len(daily_data_list) <= days

🧰 Tools

🪛 Ruff (0.11.9)

108-108: Line too long (82 > 79)

(E501)

---

110-110: Line too long (80 > 79)

(E501)

src/garth/data/body_battery.py (3)

147-150: Minor line length adjustments needed.

-            stress_chart_value_offset=response.get("stressChartValueOffset", 0),
-            stress_chart_y_axis_origin=response.get("stressChartYAxisOrigin", 0),
+            stress_chart_value_offset=response.get(
+                "stressChartValueOffset", 0
+            ),
+            stress_chart_y_axis_origin=response.get(
+                "stressChartYAxisOrigin", 0
+            ),

-            body_battery_values_array=response.get("bodyBatteryValuesArray", [])
+            body_battery_values_array=response.get(
+                "bodyBatteryValuesArray", []
+            )

🧰 Tools

🪛 Ruff (0.11.9)

147-147: Line too long (80 > 79)

(E501)

---

148-148: Line too long (81 > 79)

(E501)

---

150-150: Line too long (80 > 79)

(E501)

---

162-162: Remove redundant timedelta import.

With the suggested import consolidation at the top of the file, this local import becomes unnecessary.

-        from datetime import timedelta

---

278-278: Remove redundant timedelta import.

With the suggested import consolidation at the top of the file, this local import becomes unnecessary.

-        from datetime import timedelta

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

src/garth/data/body_battery.py (1)

155-158: Minor formatting: Address line length violations.

The static analysis correctly identifies lines that slightly exceed the 79-character limit.

-            stress_chart_value_offset=response.get("stressChartValueOffset", 0),
-            stress_chart_y_axis_origin=response.get("stressChartYAxisOrigin", 0),
+            stress_chart_value_offset=response.get(
+                "stressChartValueOffset", 0
+            ),
+            stress_chart_y_axis_origin=response.get(
+                "stressChartYAxisOrigin", 0
+            ),
-            body_battery_values_array=response.get("bodyBatteryValuesArray", [])
+            body_battery_values_array=response.get(
+                "bodyBatteryValuesArray", []
+            )

🧰 Tools

🪛 Ruff (0.11.9)

155-155: Line too long (80 > 79)

(E501)

---

156-156: Line too long (81 > 79)

(E501)

---

158-158: Line too long (80 > 79)

(E501)

src/garth/data/body_battery.py (1)

57-114: Consider caching property calculations for better performance.

The property implementations are well-structured and handle edge cases properly. However, properties like body_battery_readings are recalculated on each access, and metric properties call this multiple times, which could be inefficient for large datasets.

Consider using functools.cached_property or similar caching mechanism:

+from functools import cached_property

-    @property
+    @cached_property
     def body_battery_readings(self) -> List[BodyBatteryReading]:

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

pyproject.toml (1)

78-80: Tighten version constraints for ruff
To prevent untested major releases from breaking your CI, add an upper bound to the version specifier. For example:

[dependency-groups]
-dev = [
-    "ruff>=0.11.5",
-]
+dev = [
+    "ruff>=0.11.5,<0.12.0",
+]

src/garth/data/body_battery_events.py (1)

39-54: Improve data validation and consider performance optimization.

The property logic is correct but could be more robust:

1. Data validation: The code assumes array elements have correct types but doesn't validate them
2. Performance: The property recreates the list on every access, which could be inefficient for large datasets

Consider these improvements:

-    @property
+    @cached_property
     def body_battery_readings(self) -> List[BodyBatteryReading]:
         """Convert body battery values array to structured readings."""
         readings = []
         for values in self.body_battery_values_array or []:
             # Each reading requires 4 values: timestamp, status, level, version
-            if len(values) >= 4:
+            if len(values) >= 4:
+                try:
                     readings.append(
                         BodyBatteryReading(
                             timestamp=values[0],
                             status=values[1],
                             level=values[2],
                             version=values[3],
                         )
                     )
+                except (TypeError, ValueError):
+                    # Skip invalid readings
+                    continue
         return readings

Don't forget to import cached_property from functools.

src/garth/data/daily_body_battery_stress.py (2)

50-65: Good use of cached_property for performance optimization.

The cached property implementation is excellent for performance. However, consider adding type validation similar to the suggestion for the first file.

Consider adding error handling for invalid data:

     @cached_property
     def body_battery_readings(self) -> List[BodyBatteryReading]:
         """Convert body battery values array to structured readings."""
         readings = []
         for values in self.body_battery_values_array or []:
             # Each reading requires 4 values: timestamp, status, level, version
             if len(values) >= 4:
+                try:
                     readings.append(
                         BodyBatteryReading(
                             timestamp=values[0],
                             status=values[1],
                             level=values[2],
                             version=values[3],
                         )
                     )
+                except (TypeError, ValueError):
+                    continue
         return readings

---

67-76: Consider using cached_property for consistency and performance.

The implementation is correct, but for consistency with body_battery_readings and better performance, consider using @cached_property.

-    @property
+    @cached_property
     def stress_readings(self) -> List[StressReading]:

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

src/garth/data/body_battery/events.py (3)

1-1: Remove unused import.

The timedelta import is not used anywhere in the file.

-from datetime import date, datetime, timedelta
+from datetime import date, datetime

🧰 Tools

🪛 Ruff (0.11.9)

1-1: datetime.timedelta imported but unused

Remove unused import: datetime.timedelta

(F401)

---

88-92: Simplify datetime parsing logic.

The datetime parsing logic can be simplified by using a more direct approach.

-                    event_start_time_gmt=datetime.fromisoformat(
-                        event_data.get("eventStartTimeGmt", "").replace(
-                            "Z", "+00:00"
-                        )
-                    ),
+                    event_start_time_gmt=datetime.fromisoformat(
+                        event_data.get("eventStartTimeGmt", "").replace("Z", "+00:00")
+                    ),

Alternatively, consider using a utility function if this datetime parsing pattern is used elsewhere in the codebase.

---

81-115: Consider extracting event parsing logic.

The manual parsing of event data with multiple .get() calls could be extracted into a separate method or use a more structured approach for better maintainability.

Consider creating a helper method:

def _parse_event_data(self, event_data: dict) -> BodyBatteryEvent:
    """Parse event data dictionary into BodyBatteryEvent."""
    return BodyBatteryEvent(
        event_type=event_data.get("eventType", ""),
        event_start_time_gmt=datetime.fromisoformat(
            event_data.get("eventStartTimeGmt", "").replace("Z", "+00:00")
        ),
        timezone_offset=event_data.get("timezoneOffset", 0),
        duration_in_milliseconds=event_data.get("durationInMilliseconds", 0),
        body_battery_impact=event_data.get("bodyBatteryImpact", 0),
        feedback_type=event_data.get("feedbackType", ""),
        short_feedback=event_data.get("shortFeedback", ""),
    )

This would improve readability and make the parsing logic reusable.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

src/garth/data/body_battery/events.py (1)

64-181: Excellent validation improvements, but needs formatting.

The comprehensive validation and error handling addresses the concerns from previous reviews perfectly. The implementation handles missing data, invalid types, and malformed datetime strings robustly.

However, there are multiple line length violations flagged by static analysis.

Apply formatting to fix line length violations:

-                        logging.error(f"Missing eventStartTimeGmt in event data: {event_data}")
+                        logging.error(
+                            f"Missing eventStartTimeGmt in event data: {event_data}"
+                        )
-                        raise ValueError("eventStartTimeGmt is required but missing")
+                        raise ValueError(
+                            "eventStartTimeGmt is required but missing"
+                        )

Consider running make format as suggested in the PR comments to automatically fix all formatting issues.

🧰 Tools

🪛 Ruff (0.11.9)

99-99: Line too long (95 > 79)

(E501)

---

100-100: Line too long (85 > 79)

(E501)

---

107-107: Line too long (95 > 79)

(E501)

---

108-108: Line too long (108 > 79)

(E501)

---

113-113: Line too long (106 > 79)

(E501)

---

118-118: Line too long (109 > 79)

(E501)

---

123-123: Line too long (107 > 79)

(E501)

---

138-138: Line too long (85 > 79)

(E501)

---

139-139: Line too long (105 > 79)

(E501)

---

143-143: Line too long (87 > 79)

(E501)

---

144-144: Line too long (111 > 79)

(E501)

---

149-149: Line too long (87 > 79)

(E501)

---

150-150: Line too long (98 > 79)

(E501)

---

167-167: Line too long (95 > 79)

(E501)

---

171-171: Line too long (109 > 79)

(E501)

---

179-179: Line too long (136 > 79)

(E501)

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

tests/data/test_body_battery_data.py (1)

46-46: Remove meaningless assertion.

The assertion assert len(body_battery_data) >= 0 is always True for any list, making it meaningless as a test condition.

Apply this diff to remove the meaningless assertion:

-    # Test that we get data (may be empty if no events)
-    assert len(body_battery_data) >= 0

Or replace with a more meaningful assertion if you want to verify the structure:

-    # Test that we get data (may be empty if no events)
-    assert len(body_battery_data) >= 0
+    # Verify we get a list (may be empty if no events)
+    # Length check is implicit in isinstance(body_battery_data, list)

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

.github/workflows/ci.yml (1)

73-74: Same check for the testing group
Ensure that pip ≥ 23.2 on each runner OS supports --group testing and that the testing group is correctly declared in pyproject.toml.

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

src/garth/data/_base.py (1)

20-20: Interface change introduces flexibility but breaks consistency.

Expanding the return type to Self | list[Self] | None allows implementations to return either single instances or lists, which is necessary for Body Battery data that has multiple events per day. However, this introduces inconsistency in the interface where different implementations return different types.

Consider documenting this behavior change clearly, as it's a breaking change to the abstract interface that subclasses must handle appropriately.

Consider adding a docstring to clarify when implementations should return single instances vs. lists:

 @classmethod
 @abstractmethod
 def get(
     cls, day: date | str, *, client: http.Client | None = None
-) -> Self | list[Self] | None: ...
+) -> Self | list[Self] | None: 
+    """Get data for a specific date.
+    
+    Returns:
+        Single instance for data types with one record per day,
+        list of instances for data types with multiple records per day,
+        or None if no data exists.
+    """
+    ...

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro