Skip to content
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

Adding a progress bar #578

Merged
merged 5 commits into from
May 10, 2016
Merged

Adding a progress bar #578

merged 5 commits into from
May 10, 2016

Conversation

drvinceknight
Copy link
Member

@drvinceknight drvinceknight commented May 9, 2016

Closes #558

This is built on top of #572.

Works for both parallel and serial. Here's a gif showing it (code from https://gist.github.com/drvinceknight/2f42efc2388f41d3da7931ffc10aee6f):

941gayzwqh

@drvinceknight drvinceknight changed the title Adding a progress bar Adding a progress bar (Depends on #558) May 9, 2016
@drvinceknight drvinceknight changed the title Adding a progress bar (Depends on #558) Adding a progress bar (Depends on #572) May 9, 2016
@marcharper
Copy link
Member

Very nice :)

@@ -66,7 +68,7 @@ def setup_output_file(self, filename=None):
# Save filename for loading ResultSet later
self.filename = filename

def play(self, build_results=True, filename=None):
def play(self, build_results=True, filename=None, progress_bar=None):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If progress_bar is a boolean, why not use False as the default value?

- Progress according to chunks
- Works for both parallel and serial

Here's a gif showing how this looks: http://g.recordit.co/941gAYzwqh.gif
@drvinceknight
Copy link
Member Author

Good call @meatballs, have made that change (and also added some more tests that check if the progress bar was or wasn't created).

@drvinceknight
Copy link
Member Author

Have added a line to the docs about the progress bar @meatballs: 08a0bb5

I don't feel strongly about it. Just a thought. Happy to throw it out...

@meatballs
Copy link
Member

👍

Vince Knight wrote:

Have added a line to the docs about the progress bar @meatballs
https://github.com/meatballs: 08a0bb5
08a0bb5

I don't feel strongly about it. Just a thought. Happy to throw it out...


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#578 (comment)

@marcharper
Copy link
Member

It looks good for me, my only comment is that we might want progress bars on by default. Regardless this is 👍 from me.

@drvinceknight
Copy link
Member Author

I'm easy.

Note that with False by default when running the test suite you get 2 or 3
progress bars appear in the output. Changing it to True by default would
have a BUNCH more of them. Not the end of the world.

Any preference @meatballs?

On Tue, 10 May 2016, 15:38 Marc Harper, [email protected] wrote:

It looks good for me, my only comment is that we might want progress bars
on by default. Regardless this is [image: 👍] from me.


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#578 (comment)

@meatballs
Copy link
Member

How about True by default but the test suite passes in False?

@drvinceknight
Copy link
Member Author

That would work. I'll do that now on the train. :)

On Tue, 10 May 2016, 15:43 Owen Campbell, [email protected] wrote:

How about True by default but the test suite passes in False?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#578 (comment)

@drvinceknight
Copy link
Member Author

How about True by default but the test suite passes in False?

That's done. A few progress bar still appear (to test that it actually works and doesn't break anything) but most are turned off.

@drvinceknight drvinceknight changed the title Adding a progress bar (Depends on #572) Adding a progress bar May 10, 2016
@meatballs meatballs merged commit 6cab8c8 into master May 10, 2016
@meatballs meatballs deleted the 558 branch May 10, 2016 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants