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

Tweak progress bar docs. #3286

Merged
merged 5 commits into from
Sep 20, 2023
Merged

Tweak progress bar docs. #3286

merged 5 commits into from
Sep 20, 2023

Conversation

rodrigogiraoserrao
Copy link
Contributor

@rodrigogiraoserrao rodrigogiraoserrao commented Sep 12, 2023

There is no good reason as to why the progress bar can't be set back to its indeterminate state (and you could actually do it with code) so this removes the docstring that says that a progress bar can't go back to its indeterminate state.

Related issue: #3268
Related Discord message: https://discord.com/channels/1026214085173461072/1033754296224841768/1149742624002023594

The user in the linked Discord message reports that pb.update(total=None) “didn't change the [total] value so it doesn't work” but that's how update works:

  • pb.update(progress=None) does nothing; and
  • pb.update(advance=None) also does nothing.

The method update can only affect changes for keyword arguments that are set to a value other than None.
Setting pb.total = None works (it already worked) and the docstring for total no longer says otherwise.
I also added a test to make sure that setting total back to None works, so now this is explicitly a supported feature.

There is no good reason as to why the progress bar can't be set back to its indeterminate state (and you could actually do it with code) so this removes the docstring that says that a progress bar can't go back to its indeterminate state.

Related issue: #3268
Related Discord message: https://discord.com/channels/1026214085173461072/1033754296224841768/1149742624002023594
Copy link
Collaborator

@willmcgugan willmcgugan left a comment

Choose a reason for hiding this comment

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

You can also set the total with the update, and it look like that prohibits setting None.

@rodrigogiraoserrao
Copy link
Contributor Author

rodrigogiraoserrao commented Sep 15, 2023

You can also set the total with the update, and it look like that prohibits setting None.

It's not that it prohibits. It's the only way to design such an update method which can update multiple things at once. It's just like how update works for rich progress bars (docs) and it's written clearly that update(some_key=some_value) will only have any effect if some_value is not None, given that this applies to total, progress, and advance.

@rodrigogiraoserrao
Copy link
Contributor Author

rodrigogiraoserrao commented Sep 15, 2023

I can get rid of the method update if it's causing more confusion than doing good.

To comply with #3286 (review) we create a new type around a sentinel object and check whether we're using the sentinel before modifying the progress bar reactives.

Things that didn't quite work well:
- directly checking 'if parameter is not _sentinel:' won't satisfy type checkers because that condition doesn't restrict the type of 'parameter' to _not_ be 'UnsetParameter'.
- checking 'isinstance(parameter, float)' isn't enough because the user may call the method with an integer like '3' and then the isinstance check would fail.

- checking 'isinstance(parameter, (int, float))' works but looks a bit odd, plus it is not very general.
@willmcgugan willmcgugan merged commit 79e9f3b into main Sep 20, 2023
23 checks passed
@willmcgugan willmcgugan deleted the progress-bar-docs branch September 20, 2023 12:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants