Time in power zones for activities #316
Conversation
WalkthroughTwo new public methods were added to the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
garminconnect/__init__.py
🧰 Additional context used
📓 Path-based instructions (1)
garminconnect/**
⚙️ CodeRabbit configuration file
garminconnect/**: - prefer modern Python patterns (3.10+)
- use type hints consistently
- follow PEP 8 and modern Python conventions
- suggest performance improvements where applicable
Files:
garminconnect/__init__.py
| def get_activity_power_in_timezones(self, activity_id: str) -> dict[str, Any]: | ||
| """Return activity power in timezones.""" | ||
|
|
||
| activity_id = str(activity_id) | ||
| url = f"{self.garmin_connect_activity}/{activity_id}/powerTimeInZones" | ||
| logger.debug("Requesting Power time-in-zones for activity id %s", activity_id) | ||
|
|
||
| return self.connectapi(url) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the garminconnect/__init__.py file and check its line count
find . -name "__init__.py" -path "*/garminconnect/*" -type f 2>/dev/null | head -5
# Also check if the file exists and its line count
if [ -f "garminconnect/__init__.py" ]; then
wc -l garminconnect/__init__.py
fiRepository: cyberjunky/python-garminconnect
Length of output: 133
🏁 Script executed:
#!/bin/bash
# Read the specific lines around the methods being reviewed
sed -n '2105,2135p' garminconnect/__init__.pyRepository: cyberjunky/python-garminconnect
Length of output: 1260
🏁 Script executed:
#!/bin/bash
# Look for tests related to these methods
find . -name "*.py" -path "*/test*" -type f 2>/dev/null | head -10
# Also check for any test files that might test activity power or HR in timezones
rg -l "get_activity_power_in_timezones|get_activity_hr_in_timezones|powerTimeInZones|hrTimeInZones" --type=py -g "test*" 2>/dev/null || echo "No test files found with these patterns"Repository: cyberjunky/python-garminconnect
Length of output: 371
🏁 Script executed:
#!/bin/bash
# Look at the test file to understand how activity methods are tested
rg -A 5 "def test.*activity" tests/test_garmin.py | head -50
# Also check for any cassettes or fixtures that might contain sample data
find . -name "*.yaml" -o -name "*.json" | grep -i "cassette\|fixture" | head -10Repository: cyberjunky/python-garminconnect
Length of output: 357
🏁 Script executed:
#!/bin/bash
# Check the connectapi method to understand what it returns
rg -A 10 "def connectapi" garminconnect/__init__.py | head -30
# Also check if there are any other *_in_timezones methods to understand the pattern
rg -B 2 -A 7 "_in_timezones" garminconnect/__init__.pyRepository: cyberjunky/python-garminconnect
Length of output: 1396
🏁 Script executed:
#!/bin/bash
# Check the get_cycling_ftp method more closely - it has a union return type
sed -n '2123,2140p' garminconnect/__init__.py
# Also look for any documentation or comments about the powerTimeInZones endpoint
rg -B 5 "powerTimeInZones" garminconnect/__init__.py
# Check if there are any comments in the PR or file about return values
rg -i "empty list|power.*zone|hr.*zone" garminconnect/__init__.py | head -20Repository: cyberjunky/python-garminconnect
Length of output: 1553
🏁 Script executed:
#!/bin/bash
# Check if there's any documentation or PR comments about return values
# Look for any method that returns union types (dict or list) to see the pattern
rg -B 2 "dict\[.*\].*\|.*list" garminconnect/__init__.py | head -40
# Also check the get_cycling_ftp to see similar pattern
sed -n '2119,2133p' garminconnect/__init__.pyRepository: cyberjunky/python-garminconnect
Length of output: 849
Implementation is consistent with existing patterns.
The method correctly mirrors get_activity_hr_in_timezones and follows modern Python 3.10+ conventions. The implementation and type hint are sound—dict[str, Any] return type matches the similar activity time-in-zones method already in the codebase.
Consider enhancing the docstring for clarity: describe what "power in timezones" represents (e.g., "time spent in different power zones during an activity") to match documentation standards for similar methods.
🤖 Prompt for AI Agents
In garminconnect/__init__.py around lines 2114 to 2121, the method
get_activity_power_in_timezones has a minimal docstring; update it to a clearer
description that matches other methods (e.g., explain that it returns the time
spent in different power zones during the activity and the structure of the
returned dict), keep the signature and type hints unchanged, and ensure the
wording mirrors the style used by get_activity_hr_in_timezones for consistency.
| def get_cycling_ftp( | ||
| self, | ||
| ) -> dict[str, Any] | list[dict[str, Any]]: | ||
| """ | ||
| Return cycling Functional Threshold Power (FTP) information. | ||
| """ | ||
|
|
||
| url = f"{self.garmin_connect_biometric_url}/latestFunctionalThresholdPower/CYCLING" | ||
| logger.debug("Requesting latest cycling FTP") | ||
| return self.connectapi(url) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Get file size to ensure it's readable
wc -l garminconnect/__init__.py
# Read the get_cycling_ftp method (lines 2123-2132)
echo "=== get_cycling_ftp method (lines 2123-2132) ==="
sed -n '2120,2135p' garminconnect/__init__.py
# Read the get_lactate_threshold method (lines 1054-1060)
echo "=== get_lactate_threshold method (lines 1054-1060) ==="
sed -n '1050,1065p' garminconnect/__init__.py
# Search for other FTP methods
echo "=== Searching for other FTP methods ==="
rg -n "def get.*[Ff][Tt][Pp]" garminconnect/__init__.py
# Search for methods that normalize return values
echo "=== Searching for normalization patterns ==="
rg -n "isinstance.*list.*if.*result" garminconnect/__init__.py | head -20Repository: cyberjunky/python-garminconnect
Length of output: 1423
🏁 Script executed:
#!/bin/bash
# Find the full get_lactate_threshold method
echo "=== Looking for get_lactate_threshold method definition ==="
rg -n "def get_lactate_threshold" garminconnect/__init__.py
# Read a wider range to see the complete get_lactate_threshold
echo "=== Complete get_lactate_threshold method ==="
sed -n '1040,1100p' garminconnect/__init__.py
# Look for similar patterns - other methods that return dict | list
echo "=== Methods with union return types (dict | list) ==="
rg -n "-> dict\[str, Any\] \| list" garminconnect/__init__.py
# Check the connectapi method to understand what it returns
echo "=== Looking for connectapi method ==="
rg -n "def connectapi" garminconnect/__init__.pyRepository: cyberjunky/python-garminconnect
Length of output: 3805
🏁 Script executed:
#!/bin/bash
# Search for all get_*ftp or get_*threshold methods
echo "=== All FTP/Threshold related methods ==="
rg -n "def get.*[Ff][Tt][Pp]|def get.*[Tt]hreshold" garminconnect/__init__.py
# Get the return type of connectapi
echo "=== connectapi method and return type ==="
sed -n '294,310p' garminconnect/__init__.py
# Check if there are any other methods that return the union type dict | list
echo "=== Check return type of get_cycling_ftp and return statements ==="
sed -n '2123,2140p' garminconnect/__init__.py
# Search for "latestRunningFTP" or similar running FTP endpoints
echo "=== Search for running FTP endpoint ==="
rg -n "Running.*[Ff][Tt][Pp]|runningFtp|RunningFTP" garminconnect/__init__.py
# Look at line 1124 mentioned in review
echo "=== Context around line 1124 ==="
sed -n '1120,1130p' garminconnect/__init__.pyRepository: cyberjunky/python-garminconnect
Length of output: 2688
🏁 Script executed:
#!/bin/bash
# Check the complete get_lactate_threshold signature
echo "=== get_lactate_threshold method signature ==="
sed -n '1031,1045p' garminconnect/__init__.py
# Check if there are type hints on the return
echo "=== Search for return type hints on get_lactate_threshold ==="
rg -A 15 "def get_lactate_threshold" garminconnect/__init__.py | head -20
# Look for all methods with dict | in their return type
echo "=== Methods with pipe (|) in return type ==="
rg -n "def get.*\|" garminconnect/__init__.pyRepository: cyberjunky/python-garminconnect
Length of output: 2740
Document the union return type and clarify API response behavior.
The method correctly uses modern Python 3.10+ union syntax. However, the dict[str, Any] | list[dict[str, Any]] return type requires clarification:
-
Document when each type is returned: Specify in the docstring whether the API returns a dict or list depending on response conditions. This helps callers handle both cases appropriately.
-
Consider normalizing the return value: You could normalize to always return a dict for API consistency:
result = self.connectapi(url) if isinstance(result, list) and result: return result[0] return result if isinstance(result, dict) else {}
This simplifies caller code, though returning the raw response is also a valid design choice.
-
Enhance the docstring: Add details about the FTP data structure and clarify this returns the latest/most recent value.
🤖 Prompt for AI Agents
In garminconnect/__init__.py around lines 2123–2132, the method currently
annotates its return as dict[str, Any] | list[dict[str, Any]] but the docstring
doesn't explain when a dict vs list is returned and callers must handle both;
update the docstring to state this method returns the latest cycling FTP (most
recent value) and describe the expected dict fields and the condition under
which the API may return a list (e.g. historical entries vs single latest
entry), and then normalize the return value in the implementation by assigning
result = self.connectapi(url) and returning result[0] if result is a non-empty
list, returning result if it is a dict, and returning an empty dict otherwise so
callers always receive a dict.
1 participant
Added a API call to get time in power zones for activities.
Works for activity types: virtual_ride, cycling, indoor_ride, running (and more)
If not available returns empty list.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.