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

Progressbar object is no more an iterator #1125

Closed
saimn opened this issue Sep 27, 2018 · 4 comments
Closed

Progressbar object is no more an iterator #1125

saimn opened this issue Sep 27, 2018 · 4 comments
Milestone

Comments

@saimn
Copy link

@saimn saimn commented Sep 27, 2018

Hi,

With Click < 7.0, iterating on the bar object was possible:

In [8]: with click.progressbar(range(10)) as bar:
   ...:     next(bar)
   ...:     next(bar)
   ...:     
  [#######-----------------------------]   20%

With Click 7.0, this is broken:

In [2]: with click.progressbar(range(10)) as bar:
   ...:     next(bar)
   ...:     next(bar)
   ...:     
---------------------------------------------------------------------------
TypeError                                 Traceback (most recent call last)
<ipython-input-2-85d302084fbb> in <module>()
      1 with click.progressbar(range(10)) as bar:
----> 2     next(bar)
      3     next(bar)
      4 

TypeError: 'ProgressBar' object is not an iterator

Is it on purpose ? Seems related to 82f9c62 (#487)

@davidism

This comment has been minimized.

Copy link
Member

@davidism davidism commented Sep 27, 2018

Bisect shows 8780a76, from #706, as the origin of the issue. Prior to that, ProgressBar had a next method.

@ThiefMaster and @dsully, were involved in that PR.

@davidism davidism added this to the 7.1 milestone Sep 27, 2018
@sirosen

This comment has been minimized.

Copy link
Contributor

@sirosen sirosen commented Oct 1, 2018

Since click is a py2/py3 compatible project, we should consider using six.Iterator to make it easier to ensure that both the py2 and py3 interfaces are satisfied.

This also seems pretty easy to add a test for, based on the example given.

@davidism

This comment has been minimized.

Copy link
Member

@davidism davidism commented Oct 1, 2018

Due to various issues, we don't use six in the Pallets projects. I think there was actually an issue with streams due to six that was worked around in Click 7.

We try to implement only as much as we need in _compat.

I don't mind just adding __next__ and doing next = __next__.

@sirosen

This comment has been minimized.

Copy link
Contributor

@sirosen sirosen commented Oct 1, 2018

Due to various issues, we don't use six in the Pallets projects. I think there was actually an issue with streams due to six that was worked around in Click 7.

I'm super-curious, but I'll save looking into this until after my workday.

six.Iterator is a very, very simple class, so I certainly don't think we need to pull in six just for that.

Adding __next__ and aliasing next = __next__ on iterables is a more-than-reasonable solution. I'll try to remember to open a PR tonight.

sirosen added a commit to sirosen/click that referenced this issue Nov 6, 2018
- add a test for iterating on a progressbar with next()
- implement `next()` in terms of `iter(ProgressBar)`, which is already
  well-defined
- add a note about slow operations with progressbar to narrative docs

One of the keys to this working is that `ProgressBar.generator` is
consuming an iterable and doesn't store any important state in local
variables, making it re-entry safe.

I wasn't sure how we can best hint in the public docs that simply
walking the bar and stopping one step short of finishing will result in
the progress bar "hanging" unfinished. Noting that we work well with
things like `time.sleep()` seems like a decent way of advertising
positively something which we support *and* guiding anyone going
off-book into proper usage.

closes pallets#1125
sirosen added a commit to sirosen/click that referenced this issue Nov 10, 2018
- add a test for iterating on a progressbar with next()
- implement `next()` in terms of `iter(ProgressBar)`, which is already
  well-defined
- add a note about slow operations with progressbar to narrative docs

One of the keys to this working is that `ProgressBar.generator` is
consuming an iterable and doesn't store any important state in local
variables, making it re-entry safe.

I wasn't sure how we can best hint in the public docs that simply
walking the bar and stopping one step short of finishing will result in
the progress bar "hanging" unfinished. Noting that we work well with
things like `time.sleep()` seems like a decent way of advertising
positively something which we support *and* guiding anyone going
off-book into proper usage.

closes pallets#1125
davidism added a commit to sirosen/click that referenced this issue Feb 17, 2020
- add a test for iterating on a progressbar with next()
- implement `next()` in terms of `iter(ProgressBar)`, which is already
  well-defined
- add a note about slow operations with progressbar to narrative docs

One of the keys to this working is that `ProgressBar.generator` is
consuming an iterable and doesn't store any important state in local
variables, making it re-entry safe.

I wasn't sure how we can best hint in the public docs that simply
walking the bar and stopping one step short of finishing will result in
the progress bar "hanging" unfinished. Noting that we work well with
things like `time.sleep()` seems like a decent way of advertising
positively something which we support *and* guiding anyone going
off-book into proper usage.

closes pallets#1125
davidism added a commit to sirosen/click that referenced this issue Feb 17, 2020
- add a test for iterating on a progressbar with next()
- implement `next()` in terms of `iter(ProgressBar)`, which is already
  well-defined
- add a note about slow operations with progressbar to narrative docs

One of the keys to this working is that `ProgressBar.generator` is
consuming an iterable and doesn't store any important state in local
variables, making it re-entry safe.

I wasn't sure how we can best hint in the public docs that simply
walking the bar and stopping one step short of finishing will result in
the progress bar "hanging" unfinished. Noting that we work well with
things like `time.sleep()` seems like a decent way of advertising
positively something which we support *and* guiding anyone going
off-book into proper usage.

closes pallets#1125
@davidism davidism closed this Feb 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.