Add rename function option to return activity ID from uploaded activity #89
Conversation
also updated readme with examples
WalkthroughThe pull request introduces enhanced functionality for Garmin Connect activity management in the Changes
Assessment against linked issues
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
✨ Finishing Touches
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. 🪧 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: 7
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)garth/http.py(2 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
315-315: Expected: 1; Actual: 2
Multiple consecutive blank lines
(MD012, no-multiple-blanks)
293-293: Expected: 80; Actual: 85
Line length
(MD013, line-length)
🔇 Additional comments (1)
garth/http.py (1)
188-192: 🧹 Nitpick (assertive)Clarify the docstring for
return_id.
The docstring is clear but might benefit from explicit mention of the five-attempt retry limit and half-second exponential backoff.- (default: ``False``). This requires polling to see if Garmin - Connect has finished processing the activity, so setting this - option to ``True`` may introduce some delay + (default: ``False``). Setting ``True`` initiates a process that attempts + to retrieve the Garmin Connect activity ID over up to five polls, + each spaced out by an incremental half-second delay.Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)garth/http.py(2 hunks)
🧰 Additional context used
🪛 Markdownlint (0.37.0)
README.md
260-260: Expected: 80; Actual: 85
Line length
(MD013, line-length)
🔇 Additional comments (6)
garth/http.py (4)
4-4: Revisit selective import approach
A previous comment suggested that if you plan on using more functions from thetimemodule, it might be consistent to justimport timeinstead of onlysleep; otherwise, importing just one function is fine.
188-192: Neat addition of thereturn_idparameter
The signature cleanly communicates the new functionality. No concerns; this looks great.
231-254: Handle unexpected status codes in ID retrieval loop
This loop only checks for 202 or 201. Consider explicitly handling 4xx or 5xx to provide clearer error messages when ID retrieval fails for other reasons (e.g., permission issues).
257-292: Offer a more informative error mechanism
Raising a genericGarthHTTPErroron non-204 statuses is valid, but providing a custom exception or returning a status object can help users handle partial successes or parse more details efficiently.README.md (2)
253-258: Good documentation of the newreturn_idfeature
Adding these details clarifies how to retrieve the Garmin-internal activity ID. This is helpful for advanced usage.
294-317: Renaming workflow is clearly explained
The documentation nicely walks users through fetching the new ID and applying a rename. No issues.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (2)
README.md(1 hunks)garth/http.py(2 hunks)
🧰 Additional context used
🪛 markdownlint-cli2 (0.17.2)
README.md
260-260: Line length
Expected: 80; Actual: 85
(MD013, line-length)
🔇 Additional comments (6)
README.md (3)
263-267: Looks good overall.
The code snippet to demonstratereturn_id=Trueis straightforward and aligns with the described workflow.
268-292: Clear and helpful example.
The sample JSON response effectively shows the structure for retrievinginternalId; no issues found.
293-318: Renaming snippet is well-documented.
The explanation for fetching the ID and callingrenameis concise and clear. No issues noted.garth/http.py (3)
4-4: [Duplicate Comment] Consider importing the entiretimemodule.
As previously mentioned, if you're likely to use othertimefunctions, you might consider usingimport timeand callingtime.sleep(...)for consistency. Otherwise, keeping a single function-level import is also acceptable.
188-209: Documentation enhancement is solid.
The new docstring thoroughly explains thereturn_idparameter and potential wait time. Good job including the approximate delay.
262-297: Rename method looks fine.
The docstring is sufficient, and returning a 204 implies success. No critical concerns here.
| Using the `return_id` flag will make the code wait for Garmin to return | ||
| its internal identifier for the newly created activity, which can | ||
| be used for other methods such as renaming. The "internal ID" will | ||
| be contained within the resulting dictionary at | ||
| `result["detailedImportResult"]["successes"][0]["internalId"]`: | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Consider mentioning edge cases for empty successes array.
In practice, the "successes" array may be empty if Garmin fails to process the upload or if it’s still pending. It could be helpful to briefly clarify how users should handle a scenario in which "successes" is empty and no "internalId" is returned.
| > [!NOTE] | ||
| > Since this process waits for processing on the Garmin Connect server, be aware that | ||
| > it can make the upload process can take up to a few seconds longer than usual. | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick (assertive)
Line length exceeds markdownlint's recommended limit.
Consider splitting this note into multiple lines to comply with the 80-character guideline:
-> Since this process waits for processing on the Garmin Connect server, be aware that
+> Since this process waits for processing on the Garmin Connect server,
+> be aware that📝 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.
| > [!NOTE] | |
| > Since this process waits for processing on the Garmin Connect server, be aware that | |
| > it can make the upload process can take up to a few seconds longer than usual. | |
| > [!NOTE] | |
| > Since this process waits for processing on the Garmin Connect server, | |
| > be aware that | |
| > it can make the upload process can take up to a few seconds longer than usual. |
🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
260-260: Line length
Expected: 80; Actual: 85
(MD013, line-length)
| if return_id: | ||
| tries = 0 | ||
| # get internal activity ID from garmin connect, try five | ||
| # times with increasing waits (it takes some time for Garmin | ||
| # connect to return an ID) | ||
| while tries < 5: | ||
| wait = (tries + 1) * 0.5 | ||
| sleep(wait) | ||
| if "location" in resp.headers: | ||
| id_resp = self.request( | ||
| "GET", | ||
| "connectapi", | ||
| resp.headers["location"].replace( | ||
| "https://connectapi.garmin.com", | ||
| "", | ||
| ), | ||
| api=True | ||
| ) | ||
| if id_resp.status_code == 202: | ||
| continue | ||
| elif id_resp.status_code == 201: | ||
| result["detailedImportResult"]["successes"] = \ | ||
| id_resp.json()["detailedImportResult"]["successes"] | ||
| break | ||
| return result |
There was a problem hiding this comment.
🧹 Nitpick (assertive)
No fallback if the ID remains unavailable after five tries.
If Garmin still hasn’t assigned an ID after five attempts, the code silently returns a result without the new "internalId". Consider throwing an exception or logging a warning to alert users that the upload may not be fully processed.
| resp = self.request( | ||
| "POST", | ||
| "connectapi", | ||
| path, | ||
| method="POST", | ||
| files=files, | ||
| ) | ||
| assert result is not None, "No result from upload" | ||
| result = None if resp.status_code == 204 else resp.json() | ||
| if result is None: | ||
| raise GarthHTTPError( | ||
| msg=( | ||
| "Upload did not have expected status code of 204 " | ||
| f"(was: {resp.status_code})" | ||
| ), | ||
| error=HTTPError(), | ||
| ) | ||
|
|
There was a problem hiding this comment.
Potential confusion over status code check.
This block raises an error if resp.status_code is 204, yet the error message claims the upload “did not have expected status code of 204.” Verify whether 204 should indeed be an error or if it’s actually the expected success code. Otherwise, the error message may be misleading.
- if result is None:
- raise GarthHTTPError(
- msg=( "Upload did not have expected status code of 204 "
- f"(was: {resp.status_code})" ),
- error=HTTPError(),
- )
+ # Re-check the intended status code expectations and revise accordingly.Committable suggestion skipped: line range outside the PR's diff.
Resolves #88.
Not much to describe apart from the PR title.
I modified
upload()to useClient.requestto be able to access the "location" response header, which contains the URL needed to fetch the activity ID of the uploaded file. This is not returned initially when the file is uploaded, as it takes Garmin some time to process the activity. I implemented a simple polling process to check if the ID is ready, backing off by half a second with each try. If it doesn't return the value within 5 tries, it gives up.The headers include with the
rename()implementation are what I found to be the minimum required, but I'm not very experienced with the Garmin API, so maybe you have a better implementation.I also edited the readme to describe the new functionality with a demo.
Summary by CodeRabbit