-
Notifications
You must be signed in to change notification settings - Fork 12
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
Better progress logger for stor #79
base: master
Are you sure you want to change the base?
Conversation
Replace the existing progress logger with tqdm, a library that makes elegant progress bars that calculate rate of transfer, time remaining, and also look nice on the CLI (i.e., stays on one line and doesn't gum up the entire works). A future enhancement would be to make the progress logger configurable, but I figure for now just showing a better one is a bonus. How this was tested: 1. Tested stor cp -r and rm -r with various combinatiosn 2. Ensure passes all unit tests. Sem-Ver: feature
@@ -74,6 +74,7 @@ | |||
""" | |||
import argparse | |||
import copy | |||
from collections import OrderedDict |
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.
AFAIK this import is unused
logger = progress_logger | ||
|
||
def add_result(self, result): | ||
self.update_progress(num_objects=1, addl_bytes=os.path.getszie(result['dest'])) |
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.
Is this a typo? getszie
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.
hmmm... maybe I didn't let the s3 version bubble up long enough..
This functionality is semi-covered by the integration tests (which call copytree and friends). I think I can test it by checking for the calls to update_progress and/or grabbing the progress logger after running. |
A few typos to fix. Overall idea SGTM. |
Replace the existing progress logger with tqdm, a library that makes
elegant progress bars that calculate rate of transfer, time remaining,
and also look nice on the CLI (i.e., stays on one line and doesn't gum
up the entire works). A future enhancement would be to make the progress
logger configurable, but I figure for now just showing a better one is a
bonus.
How this was tested:
I'm calling this a feature break: it does not break any existing APIs and the progress logger was never a public method exposed to users.
cc @kyleabeauchamp @wesleykendall @pkaleta @krhaas - if any of you have time to review that'd be awesome