Add rename function option to return activity ID from uploaded activity #89
Conversation
also updated readme with examples
|
Warning Rate limit exceeded@jat255 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 27 minutes and 16 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
WalkthroughThe pull request introduces enhanced functionality for Garmin Connect activity management in the Changes
Assessment against linked issues
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: 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.
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