Added activity file selector to demo, removed black #295
Conversation
WalkthroughReplaces Black with Ruff in pre-commit tooling, updates demo.py to use a glob and interactive GPX selection with expanded HTTP error handling, adjusts CI to gate coverage upload on an actual report, removes a pdm-specific Python path from pyproject, updates coderabbit profile, and clears outputs in a docs notebook and formatting in README. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as demo.py
participant FS as FileSystem
participant API as Server
User->>CLI: run upload command
CLI->>FS: glob "test_data/*.gpx"
FS-->>CLI: list of GPX files
alt no files
CLI-->>User: "No GPX files found" & exit
else files found
CLI-->>User: display numbered list & prompt
User->>CLI: choose index
alt invalid index
CLI-->>User: "Invalid selection" & exit
else valid index
CLI->>API: POST upload(selected_file)
alt success (2xx)
API-->>CLI: success
CLI-->>User: "Upload successful"
else error (400/401/409/413/422/429)
API-->>CLI: error
CLI-->>User: specific error message
end
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (6)
💤 Files with no reviewable changes (1)
⏰ 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). (8)
🔇 Additional comments (5)
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: 1
🧹 Nitpick comments (2)
demo.py (2)
73-73: Clarify config usage or update the glob pattern.The
activityfileconfig value is now a glob pattern ("test_data/*.gpx"), but the actual file selection logic at line 1294 uses a hardcoded pattern instead of referencingconfig.activityfile. This inconsistency can lead to confusion.Apply this diff to use the config value consistently:
- gpx_files = glob.glob("test_data/*.gpx") + gpx_files = glob.glob(config.activityfile)Alternatively, if you prefer the hardcoded approach, update the config comment to reflect that this value is for display purposes only.
1289-1318: Remove redundant import and file existence check.Two minor inefficiencies:
osis already imported at line 23, so the import on line 1290 is redundant.- The file existence check on lines 1316-1318 is unnecessary since
selected_filewas just chosen fromglob.glob()results, which only returns existing files.Apply this diff to remove redundancies:
"""Upload activity data from file.""" import glob - import os try: # List all .gpx files in test_data gpx_files = glob.glob("test_data/*.gpx") if not gpx_files: print("❌ No .gpx files found in test_data directory.") print("ℹ️ Please add GPX files to test_data before uploading.") return print("Select a GPX file to upload:") for idx, fname in enumerate(gpx_files, 1): print(f" {idx}. {fname}") while True: try: choice = int(input(f"Enter number (1-{len(gpx_files)}): ")) if 1 <= choice <= len(gpx_files): selected_file = gpx_files[choice - 1] break else: print("Invalid selection. Try again.") except ValueError: print("Please enter a valid number.") print(f"📤 Uploading activity from file: {selected_file}") - if not os.path.exists(selected_file): - print(f"❌ File not found: {selected_file}") - return result = api.upload_activity(selected_file)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.pre-commit-config.yaml(1 hunks)demo.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
demo.py (1)
garminconnect/__init__.py (1)
upload_activity(1648-1700)
⏰ 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). (4)
- GitHub Check: test (3.11)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
🔇 Additional comments (2)
.pre-commit-config.yaml (1)
21-26: LGTM!The removal of Black in favor of Ruff is well-documented with a clear inline comment explaining the rationale. This prevents formatting conflicts and consolidates linting and formatting under a single tool.
demo.py (1)
1336-1412: LGTM! Comprehensive error handling.The expanded HTTP error handling provides excellent user experience with:
- Specific messages for common status codes (409, 413, 422, 400, 401, 429)
- Helpful troubleshooting tips for each scenario
- Clear explanations of what went wrong
This will greatly improve debugging and user guidance when uploads fail.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
demo.py (1)
1316-1327: Critical: Activity uploaded twice.This issue was already flagged in a previous review. The activity is uploaded twice:
- Line 1316:
result = api.upload_activity(selected_file)- Lines 1320-1325:
call_and_display(api.upload_activity, selected_file, ...)uploads it againRemove the direct
api.upload_activity(selected_file)call on line 1316 and letcall_and_displayperform the upload. Capture the return value fromcall_and_displayto determine success/failure.
🧹 Nitpick comments (2)
demo.py (2)
73-73: Consider using config.activityfile or removing this config change.The updated glob pattern
"test_data/*.gpx"on this line is never used—line 1293 hardcodes the same pattern directly inglob.glob("test_data/*.gpx"). Either referenceconfig.activityfilein the upload function, or revert this config change if it's not needed.Apply this diff to use the config variable:
- gpx_files = glob.glob("test_data/*.gpx") + gpx_files = glob.glob(config.activityfile)
1332-1408: Consider consolidating duplicate error handling logic.The error messages for status codes 409, 413, 422, 400, 401, and 429 are duplicated between the
requests.exceptions.HTTPErrorblock (lines 1332-1364) and the genericExceptionblock (lines 1374-1408). While this works correctly, it increases maintenance burden when messages need updating.Consider extracting the status code → error message mapping into a helper function or dictionary:
def get_upload_error_message(status_code: int) -> tuple[str, str]: """Returns (error_message, hint) for given HTTP status code.""" error_map = { "Activity already exists: This activity has already been uploaded to Garmin Connect", "Try modifying the activity timestamps or creating a new activity file" ), "File too large: The activity file exceeds Garmin Connect's size limit", "Try compressing the file or reducing the number of data points" ), # ... other mappings } return error_map.get(status_code, (f"HTTP Error {status_code}", "Please try again"))Then use it in both exception handlers to reduce duplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
demo.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
demo.py (1)
garminconnect/__init__.py (1)
upload_activity(1648-1700)
⏰ 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). (2)
- GitHub Check: test (3.10)
- GitHub Check: test (3.10)
🔇 Additional comments (3)
demo.py (3)
1289-1290: LGTM!The
globimport is appropriate for the new file selection functionality.
1292-1312: LGTM!The interactive file selection logic is well-implemented with proper input validation and clear user feedback. The flow handles edge cases (no files, invalid input, out-of-range selection) appropriately.
1314-1314: LGTM!The messages correctly reference
selected_fileinstead of the previous staticconfig.activityfile, which is appropriate for the new interactive file selection workflow.Also applies to: 1327-1327, 1330-1330
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
demo.py (1)
1323-1402: Redundant error handling—call_and_displayalready handles HTTP errors.
call_and_display(line 1316) wrapssafe_api_call(lines 946-1029), which already catchesGarthHTTPErrorand handles status codes 400, 401, 403, 404, 429, 500, 503 with user-friendly messages. The explicitexceptblocks forrequests.exceptions.HTTPError(lines 1326-1358) and the genericExceptionblock with string matching (lines 1368-1402) duplicate this logic and are largely unreachable, sinceapi.upload_activityraisesGarthHTTPError, notrequests.exceptions.HTTPError.This redundancy can lead to double error reporting or maintenance burden as error handling logic diverges between the two layers.
Simplify by removing the redundant HTTP error handling and keeping only the library-specific exceptions:
- except FileNotFoundError: - print(f"❌ File not found: {selected_file if selected_file else 'No file selected'}") - print("ℹ️ Please ensure the activity file exists in the current directory") - except requests.exceptions.HTTPError as e: - if e.response.status_code == 409: - print( - "⚠️ Activity already exists: This activity has already been uploaded to Garmin Connect" - ) - print("ℹ️ Garmin Connect prevents duplicate activities from being uploaded") - print( - "💡 Try modifying the activity timestamps or creating a new activity file" - ) - elif e.response.status_code == 413: - print( - "❌ File too large: The activity file exceeds Garmin Connect's size limit" - ) - print("💡 Try compressing the file or reducing the number of data points") - elif e.response.status_code == 422: - print( - "❌ Invalid file format: The activity file format is not supported or corrupted" - ) - print("ℹ️ Supported formats: FIT, GPX, TCX") - print("💡 Try converting to a different format or check file integrity") - elif e.response.status_code == 400: - print("❌ Bad request: Invalid activity data or malformed file") - print( - "💡 Check if the activity file contains valid GPS coordinates and timestamps" - ) - elif e.response.status_code == 401: - print("❌ Authentication failed: Please login again") - print("💡 Your session may have expired") - elif e.response.status_code == 429: - print("❌ Rate limit exceeded: Too many upload requests") - print("💡 Please wait a few minutes before trying again") - else: - print(f"❌ HTTP Error {e.response.status_code}: {e}") except GarminConnectAuthenticationError as e: print(f"❌ Authentication error: {e}") print("💡 Please check your login credentials and try again") except GarminConnectConnectionError as e: print(f"❌ Connection error: {e}") print("💡 Please check your internet connection and try again") except GarminConnectTooManyRequestsError as e: print(f"❌ Too many requests: {e}") print("💡 Please wait a few minutes before trying again") - except Exception as e: - error_str = str(e) - if "409 Client Error: Conflict" in error_str: - print( - "⚠️ Activity already exists: This activity has already been uploaded to Garmin Connect" - ) - print("ℹ️ Garmin Connect prevents duplicate activities from being uploaded") - print( - "💡 Try modifying the activity timestamps or creating a new activity file" - ) - elif "413" in error_str and "Request Entity Too Large" in error_str: - print( - "❌ File too large: The activity file exceeds Garmin Connect's size limit" - ) - print("💡 Try compressing the file or reducing the number of data points") - elif "422" in error_str and "Unprocessable Entity" in error_str: - print( - "❌ Invalid file format: The activity file format is not supported or corrupted" - ) - print("ℹ️ Supported formats: FIT, GPX, TCX") - print("💡 Try converting to a different format or check file integrity") - elif "400" in error_str and "Bad Request" in error_str: - print("❌ Bad request: Invalid activity data or malformed file") - print( - "💡 Check if the activity file contains valid GPS coordinates and timestamps" - ) - elif "401" in error_str and "Unauthorized" in error_str: - print("❌ Authentication failed: Please login again") - print("💡 Your session may have expired") - elif "429" in error_str and "Too Many Requests" in error_str: - print("❌ Rate limit exceeded: Too many upload requests") - print("💡 Please wait a few minutes before trying again") - else: - print(f"❌ Unexpected error uploading activity: {e}") - print("💡 Please check the file format and try again")If you need custom handling for specific HTTP status codes not covered by
safe_api_call, extend the central error handler instead of duplicating logic here.
🧹 Nitpick comments (1)
demo.py (1)
1289-1289: Move import to module level.Importing
globinside the function works but deviates from Python conventions. Module-level imports improve readability and make dependencies explicit.Move this import to the top of the file (around line 20) with other standard library imports:
import datetime import json import logging import os import sys +import glob from contextlib import suppressThen remove line 1289.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
demo.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
demo.py (1)
garminconnect/__init__.py (1)
upload_activity(1648-1700)
⏰ 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). (3)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (1)
demo.py (1)
1316-1321: Good fix—activity upload no longer duplicated.The previous review flagged that the activity was uploaded twice. The current code correctly calls
api.upload_activityonly once viacall_and_display, resolving that issue.
| self.activityfile = ( | ||
| "test_data/sample_activity.gpx" # Supported file types: .fit .gpx .tcx | ||
| ) | ||
| self.activityfile = "test_data/*.gpx" # Supported file types: .fit .gpx .tcx |
There was a problem hiding this comment.
config.activityfile is no longer used in the upload flow.
The glob pattern is defined here but never referenced in upload_activity_file (lines 1287-1403), which now performs its own file discovery via glob.glob("test_data/*.gpx"). Additionally, the menu description at line 227 displays config.activityfile, which now shows a glob pattern instead of a single file path, potentially confusing users.
Consider either:
- Remove
self.activityfilefrom Config if it's truly unused, or - Use
config.activityfileas the glob pattern inupload_activity_filefor consistency:
- gpx_files = glob.glob("test_data/*.gpx")
+ gpx_files = glob.glob(config.activityfile)And update the menu description at line 227 to be more accurate:
- "desc": f"Upload activity data from {config.activityfile}",
+ "desc": "Upload activity data (interactive file selection)",📝 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.
| self.activityfile = "test_data/*.gpx" # Supported file types: .fit .gpx .tcx | |
| # In upload_activity_file (replace hard-coded glob with config) | |
| gpx_files = glob.glob(config.activityfile) |
| self.activityfile = "test_data/*.gpx" # Supported file types: .fit .gpx .tcx | |
| # In the menu definition (make description generic) | |
| "desc": "Upload activity data (interactive file selection)", |
| while True: | ||
| try: | ||
| choice = int(input(f"Enter number (1-{len(gpx_files)}): ")) | ||
| if 1 <= choice <= len(gpx_files): | ||
| selected_file = gpx_files[choice - 1] | ||
| break | ||
| else: | ||
| print("Invalid selection. Try again.") | ||
| except ValueError: | ||
| print("Please enter a valid number.") | ||
|
|
||
| # Upload the activity | ||
| result = api.upload_activity(config.activityfile) | ||
| print(f"📤 Uploading activity from file: {selected_file}") | ||
|
|
||
| if result: | ||
| print("✅ Activity uploaded successfully!") | ||
| call_and_display( | ||
| api.upload_activity, | ||
| config.activityfile, | ||
| method_name="upload_activity", | ||
| api_call_desc=f"api.upload_activity({config.activityfile})", | ||
| ) | ||
| else: | ||
| print(f"❌ Failed to upload activity from {config.activityfile}") | ||
| call_and_display( | ||
| api.upload_activity, | ||
| selected_file, | ||
| method_name="upload_activity", | ||
| api_call_desc=f"api.upload_activity({selected_file})", | ||
| ) | ||
|
|
||
| except FileNotFoundError: | ||
| print(f"❌ File not found: {config.activityfile}") | ||
| print(f"❌ File not found: {selected_file}") |
There was a problem hiding this comment.
Risk of UnboundLocalError if exception occurs before file selection.
The try block starts at line 1291, but selected_file is only assigned at line 1307 inside the while True loop. If an exception occurs before the user completes the selection (e.g., during glob.glob() or input()), the except FileNotFoundError handler at line 1323 will reference an undefined selected_file, causing an UnboundLocalError.
Initialize selected_file before the selection loop:
try:
+ selected_file = None # Initialize before potential exceptions
# List all .gpx files in test_data
gpx_files = glob.glob("test_data/*.gpx")Then update the error message to handle the case where no file was selected:
except FileNotFoundError:
- print(f"❌ File not found: {selected_file}")
+ print(f"❌ File not found: {selected_file if selected_file else 'No file selected'}")
print("ℹ️ Please ensure the activity file exists in the current directory")📝 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.
| while True: | |
| try: | |
| choice = int(input(f"Enter number (1-{len(gpx_files)}): ")) | |
| if 1 <= choice <= len(gpx_files): | |
| selected_file = gpx_files[choice - 1] | |
| break | |
| else: | |
| print("Invalid selection. Try again.") | |
| except ValueError: | |
| print("Please enter a valid number.") | |
| # Upload the activity | |
| result = api.upload_activity(config.activityfile) | |
| print(f"📤 Uploading activity from file: {selected_file}") | |
| if result: | |
| print("✅ Activity uploaded successfully!") | |
| call_and_display( | |
| api.upload_activity, | |
| config.activityfile, | |
| method_name="upload_activity", | |
| api_call_desc=f"api.upload_activity({config.activityfile})", | |
| ) | |
| else: | |
| print(f"❌ Failed to upload activity from {config.activityfile}") | |
| call_and_display( | |
| api.upload_activity, | |
| selected_file, | |
| method_name="upload_activity", | |
| api_call_desc=f"api.upload_activity({selected_file})", | |
| ) | |
| except FileNotFoundError: | |
| print(f"❌ File not found: {config.activityfile}") | |
| print(f"❌ File not found: {selected_file}") | |
| try: | |
| selected_file = None # Initialize before potential exceptions | |
| # List all .gpx files in test_data | |
| gpx_files = glob.glob("test_data/*.gpx") | |
| while True: | |
| try: | |
| choice = int(input(f"Enter number (1-{len(gpx_files)}): ")) | |
| if 1 <= choice <= len(gpx_files): | |
| selected_file = gpx_files[choice - 1] | |
| break | |
| else: | |
| print("Invalid selection. Try again.") | |
| except ValueError: | |
| print("Please enter a valid number.") | |
| print(f"📤 Uploading activity from file: {selected_file}") | |
| call_and_display( | |
| api.upload_activity, | |
| selected_file, | |
| method_name="upload_activity", | |
| api_call_desc=f"api.upload_activity({selected_file})", | |
| ) | |
| except FileNotFoundError: | |
| print(f"❌ File not found: {selected_file if selected_file else 'No file selected'}") | |
| print("ℹ️ Please ensure the activity file exists in the current directory") |
🤖 Prompt for AI Agents
In demo.py around lines 1303 to 1324, selected_file is only set inside the
user-selection loop which can lead to an UnboundLocalError in the
FileNotFoundError except block if an exception occurs earlier; initialize
selected_file to None before the selection loop (or at the start of the
enclosing try) and change the except FileNotFoundError handler to check if
selected_file is None and print a generic "❌ File not found or no file selected"
message, otherwise print the specific filename, ensuring the handler never
references an uninitialized variable.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
demo.py (2)
73-73: Menu description should be updated for the glob pattern.The glob pattern is correctly used at line 1293, but the menu description at line 227 displays
config.activityfilewhich now shows"test_data/*.gpx"instead of describing the interactive selection feature.Update the menu description to better reflect the interactive selection:
"6": { - "desc": f"Upload activity data from {config.activityfile}", + "desc": "Upload activity data (interactive file selection)", "key": "upload_activity", },
1291-1324: Risk ofUnboundLocalErrorif exception occurs before file selection.
selected_fileis only assigned at line 1307 inside thewhile Trueloop. If an exception occurs before this assignment (e.g., duringglob.glob()at line 1293 orinput()at line 1305), theexcept FileNotFoundErrorhandler at line 1324 will reference an undefined variable, causing anUnboundLocalError.Initialize
selected_filebefore the selection logic:try: + selected_file = None # Initialize before potential exceptions # List all .gpx files in test_data gpx_files = glob.glob(config.activityfile)Then update the error handler to handle
None:except FileNotFoundError: - print(f"❌ File not found: {selected_file}") + if selected_file: + print(f"❌ File not found: {selected_file}") + else: + print("❌ File not found: No file was selected") print("ℹ️ Please ensure the activity file exists in the current directory")
🧹 Nitpick comments (2)
demo.py (2)
1289-1289: Moveimport globto module level.The
globmodule is a standard library and should be imported at the top of the file with other imports for better code organization and consistency.Move the import to line 34 (after other imports):
from garminconnect import ( Garmin, GarminConnectAuthenticationError, GarminConnectConnectionError, GarminConnectTooManyRequestsError, ) +import globAnd remove it from line 1289:
def upload_activity_file(api: Garmin) -> None: """Upload activity data from file.""" - import glob - try:
1326-1402: Consider consolidating duplicate error handling logic.The error handling for HTTP status codes (409, 413, 422, 400, 401, 429) is duplicated between the
requests.exceptions.HTTPErrorhandler (lines 1326-1357) and the genericExceptionhandler (lines 1370-1402). This creates maintenance overhead as error messages must be updated in two places.Consider extracting the error message logic into a helper function:
def _format_upload_error(status_code: int, error_str: str) -> tuple[str, str]: """Return (emoji, message) for upload error status codes.""" if status_code == 409 or "409" in error_str: return "⚠️", "Activity already exists: This activity has already been uploaded to Garmin Connect" elif status_code == 413 or "413" in error_str: return "❌", "File too large: The activity file exceeds Garmin Connect's size limit" # ... etc return "❌", f"Unexpected error uploading activity: {error_str}" # Then use it in both handlers: except requests.exceptions.HTTPError as e: emoji, msg = _format_upload_error(e.response.status_code, str(e)) print(f"{emoji} {msg}") # print tips...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
demo.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
demo.py (1)
garminconnect/__init__.py (1)
upload_activity(1648-1700)
⏰ 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). (5)
- GitHub Check: test (3.12)
- GitHub Check: test (3.13)
- GitHub Check: test (3.10)
- GitHub Check: test (3.12)
- GitHub Check: test (3.10)
🔇 Additional comments (1)
demo.py (1)
1316-1321: Confirm: Duplicate upload issue is resolved.The previous review flagged a duplicate upload where
api.upload_activitywas called twice. This has been correctly fixed - the activity is now uploaded only once viacall_and_display.
Summary by CodeRabbit
New Features
Chores