Improve follow_redirect() so that it has a singular checkpoint for limiting number of attempts and clearer logic#1634
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the follow_redirect() function to improve clarity in its retry logic and error handling.
- Updates exception handling to raise FailedToConnectError with more detailed messages.
- Adjusts test expectations in dandi/tests/test_dandiarchive.py to match the new error types and messages.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| dandi/tests/test_dandiarchive.py | Updates the expected exception and regex for the FailedToConnectError. |
| dandi/dandiarchive.py | Refines the retry logic and error messaging in follow_redirect(). |
Comments suppressed due to low confidence (2)
dandi/dandiarchive.py:897
- [nitpick] Consider renaming the loop variable 'i' to a more descriptive name like 'attempt' to improve code clarity.
for i in range(retries):
dandi/tests/test_dandiarchive.py:446
- Ensure that the regex pattern in the test fully captures both variants of the error message produced in follow_redirect(), considering the additional status code details now included.
with pytest.raises(FailedToConnectError, match=r"failed with \d+ attempts"):
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1634 +/- ##
==========================================
+ Coverage 88.77% 88.84% +0.07%
==========================================
Files 82 82
Lines 11434 11439 +5
==========================================
+ Hits 10150 10163 +13
+ Misses 1284 1276 -8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@yarikoptic Should I add a patch label to this PR. It is mostly refactoring. |
|
thanks, will review later. If you could make title more descriptive (as to why/what reorganization brings) - would be good since it would be included in the changelog at the end. |
follow_redirect()follow_redirect() so that it has a singular checkpoint for limiting number of attempts
follow_redirect() so that it has a singular checkpoint for limiting number of attemptsfollow_redirect() so that it has a singular checkpoint for limiting number of attempts and clearer logic
452b721 to
71b4f90
Compare
Overall, this commit rewrites `follow_redirect()` to bring the following improvements. 1. Make logic more clear 2. Consolidate checking of attempt index with max allow attempts into one single point. Eliminating the checks of `retry < retries` inside the `for retry in range(retries + 1)` 3. move code that doesn't need to be repeated out of the for-loop. 4. Repackage the `requests.ConnectionError` into a `FailedToConnectError` to offer better context 5. Rename `retries` in to `max_attempts` to better convey the purpose of the value 6. Correct the docstring to indicate that `FailedToConnectError` can be raised when number of allowed attempts have been exhausted.
71b4f90 to
22910d8
Compare
This PR is a follow up to #1632. It further improves
follow_redirect()by implementing more clear logic. Particularly it uses singular a checkpoint for limiting number of attempts . Please see the commit message for more details.