-
Notifications
You must be signed in to change notification settings - Fork 115
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
There would be one case where this wouldn't work #768
Conversation
Reviewer's Guide by SourceryThis pull request addresses an edge case in the download progress update logic by introducing a new variable ('size') to handle scenarios where accumulated_size has been reset to zero. The change refactors the condition in the progress update check to rely on the new 'size' variable. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @ericcurtin - I've reviewed your changes - here's some feedback:
Overall Comments:
- The final condition checks 'if size' but 'size' is never updated, suggesting an unintended switch from accumulated_size to size.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
2f9b998
to
2956111
Compare
ramalama/http_client.py
Outdated
if accumulated_size: | ||
self.update_progress(accumulated_size) | ||
if do_final_progress: | ||
if accumulated_size: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This makes no sense, to add the option.
Why not do
if accumulated_size > 0:
You reset the accumalated_size option right after the do_final_progress.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm trying to remember what I was fixing here. I'll change this to a one-liner:
if accumulated_size > 0:
like you suggested, although that shouldn't make a difference either.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah it's the "\033[K" outside the if block that's the main fix...
I dislike python's indenting as '{' '}' sometimes, makes things like this not so obvious :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wait sorry this does make sense... It's just not clear, I'll do it a different way that's more clear
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So you can break out with if not data:
when you have accumalated size and do not want to update_progress?
2956111
to
03afac4
Compare
When accumulated_size has just been refreshed to zero. Also check size is greater than zeor to avoid division by zero. Signed-off-by: Eric Curtin <[email protected]>
03afac4
to
ad4300f
Compare
LGTM Much better. Merge when tests pass. |
When accumulated_size has just been refreshed to zero
Summary by Sourcery
Bug Fixes: