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

Outdated documentation for Spinner #11004

Closed
embray opened this issue Nov 8, 2020 · 5 comments · Fixed by #11046 or #11772
Closed

Outdated documentation for Spinner #11004

embray opened this issue Nov 8, 2020 · 5 comments · Fixed by #11046 or #11772
Labels
API change PRs and issues that change an existing API, possibly requiring a deprecation period Bug Docs Effort-low good first issue Issues that are well-suited for new contributors Package-novice utils

Comments

@embray
Copy link
Member

embray commented Nov 8, 2020

Description

The current docstring in the Spinner class demonstrates using s.next() to update the spinner. But this is a Python 2-ism that no longer exists for iterators in Python 3. Instead the correct documentation would be next(s).

Additionally, I think it's confusing that the API for Spinner is so different from ProgressBar and ProgressBarOrSpinner. In the latter two cases, their __enter__ method (when using with ProgressBar(...) as bar: returns the ProgressBar(OrSpinner) instance itself, which is iterable itself. It also provides an update() method which can be called to update the progress bar. Whereas with Spinner(...) as s: does not return the Spinner instance itself, but rather a generator.

It should be easy to update the API for Spinner (make it iterable, make __enter__ return the Spinner instance, and add an update() method) so that it is more like ProgressBar.

I would suggest making the updates to Spinner and fixing the documentation at the same time.

@embray embray added Docs Priority-Low Bug Effort-low utils Package-novice good first issue Issues that are well-suited for new contributors API change PRs and issues that change an existing API, possibly requiring a deprecation period labels Nov 8, 2020
@i-am-b-soto
Copy link
Contributor

I'm on it

@i-am-b-soto
Copy link
Contributor

i-am-b-soto commented Nov 9, 2020

So. If we make the spinner iterable, It doesn't have a stop condition. So if the user decides to make an adventerous loop (like below) we will not go to space today

for s in console.Spinner("Reticulating splines", file=stream, chars=chars):
        foo_bar()

Perhaps it's better to just rewrite the generator into an update() function (while leaving the __enter__ __exit__ methods)? Or should we not worry about this use case.

@embray
Copy link
Member Author

embray commented Nov 9, 2020

That's a good point and exposes another inconsistency between Spinner and ProgressBar, where the latter can take a length/iterator as an argument. This makes sense since a progress bar is more for something with a known amount of work, while a spinner is for an unknown amount of work. Though sometimes iterators are non-finite, and it would be useful to wrap such an iterator in a spinner.

That said, your for loop example is still useful (though it would also be used with a with statement to properly cancel the spinner when done, like:

with Spinner('message') as spin:
    for _ in spin:
        # do stuff
        break

This is equivalent to having a while True: loop that you eventually have to break out of, except it updates the spinner while looping.

Meanwhile having an update() method would just be equivalent to calling next(spinner).

Basically this would be matter of:

  • change Spinner.__init__ so it does self._iter = self._iterator() (or self._silent_iterator)
  • change __enter__ to just return self and also add __iter__ which does the same (return self)
  • add __next__ which takes next(self._iter)
  • add update() which calls next(self)
  • update the examples in the documentation

@pllim
Copy link
Member

pllim commented May 21, 2021

#11046 was reverted in #11771 due to mysterious RTD timeout, so I am re-opening this issue.

@embray
Copy link
Member Author

embray commented May 21, 2021

I swear this issue is cursed.

pllim added a commit that referenced this issue Jun 1, 2021
…te_spinner_keep_enum

Update Spinner API, fix #11004
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
API change PRs and issues that change an existing API, possibly requiring a deprecation period Bug Docs Effort-low good first issue Issues that are well-suited for new contributors Package-novice utils
Projects
None yet
3 participants