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 iteration to ProgressBar to make it a more useful utility #10813

Merged
merged 2 commits into from Oct 5, 2017

Conversation

@henryiii
Copy link
Contributor

@henryiii henryiii commented Sep 18, 2017

As discussed in #10812, this simply adds the ability to iterate over a ProgessBar (no other API changes). Added a test and a mention in the relevant example.

@mariusvniekerk

This comment has been minimized.

Copy link
Contributor

@mariusvniekerk mariusvniekerk commented on IPython/core/display.py in 84bcdc3 Sep 19, 2017

I'd avoid this and instead increment after returning in the next.

"""Returns current value and increments display by one."""
self.progress += 1
if self.progress < self.total:
return self.progress

This comment has been minimized.

@mariusvniekerk

mariusvniekerk Sep 19, 2017
Contributor

Increment afterwards, that way you don't need to initialize to - 1.

This comment has been minimized.

@henryiii

henryiii Sep 21, 2017
Author Contributor

This is just a function, not a generator, so anything after the return will not run. You have to either have a -1 initial value (which is never printed), or you have to add a new variable to tell the difference between before-iteration 0 and iteration 0. You also really want the final iteration to be 4/5, to match the range, and to provide a visual indication that it's finished by changing to 5/5.

nt.assert_in('{0}/5'.format(i), out)
out, err = capsys.readouterr()
nt.assert_in('5/5', out)

This comment has been minimized.

@henryiii

henryiii Sep 21, 2017
Author Contributor

Oops, I guess this is nose running nose tests, not pytest running nose tests.

The nose project hasn't receive a commit in about a year, I think pytest would be a good idea :) I'll fix this.

@henryiii
Copy link
Contributor Author

@henryiii henryiii commented Sep 22, 2017

Is there a way to test in a normal terminal environment? It seems that the tests run in a environment where display is not supported.

@takluyver
Copy link
Member

@takluyver takluyver commented Sep 27, 2017

I don't think it's the environment that's involved there: it looks like CapturingDisplayPublisher (which is used by capture_output) hasn't been updated with the transient= keyword argument that the regular DisplayPublisher has. I guess you'll need to update that, or use capture_output(display=False) and let it capture the output on stdout.

@henryiii
Copy link
Contributor Author

@henryiii henryiii commented Sep 27, 2017

I've used the display=False method to make sure this works for IPython 5 or 6, and separately patched the display issue for IPython 5 and 6.

@takluyver
Copy link
Member

@takluyver takluyver commented Sep 28, 2017

@mariusvniekerk @Carreau are you happy with this?

Copy link
Contributor

@mariusvniekerk mariusvniekerk left a comment

Looks good

@Carreau Carreau added this to the 5.6 milestone Oct 5, 2017
@Carreau
Copy link
Member

@Carreau Carreau commented Oct 5, 2017

+1, I'm guessing backport to have identical functionality on Python2.

@henryiii
Copy link
Contributor Author

@henryiii henryiii commented Oct 5, 2017

Yes, it should be in both.

@Carreau Carreau merged commit 36ab971 into ipython:master Oct 5, 2017
4 checks passed
4 checks passed
codecov/patch 100% of diff hit (target 0%)
Details
codecov/project 67.04% (+0.02%) compared to e741a07
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
meeseeksdev bot pushed a commit that referenced this pull request Oct 5, 2017
takluyver added a commit that referenced this pull request Oct 11, 2017
Backport PR #10813 on branch 5.x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants