piotrmurach / tty-progressbar

Display a single or multiple progress bars in the terminal.
https://ttytoolkit.org
MIT License
421 stars 24 forks source link

ProgressBar#current= supports indeterminate progress bars too #56

Open alexcwatt opened 1 year ago

alexcwatt commented 1 year ago

Describe the change

I noticed that #current= failed for indeterminate progress bars. The primary test I added would error like this:

     ArgumentError:
       comparison of NilClass with 5 failed

I added a new ArgumentError guard to also preserve the existing, desirable, behavior of not supporting #current = nil, and a test to cover this. Note that current = nil was already raising ArgumentError in both the determinate and indeterminate cases (comparison of Integer with nil failed, comparison of NilClass with 0 failed, etc.); now the error message is one that we set.

Why are we doing this?

As far as I can tell, there's no reason that current = shouldn't work for indeterminate progress bars. It seems that indeterminate progress bars are meant to have a current value, and can have it by calling .advance, so adding support seems consistent with the rest of the library.

Benefits

It's quite nice to be able to set current progress for indeterminate progress bars!

Drawbacks

I don't have any to highlight.

Requirements

alexcwatt commented 1 year ago

@piotrmurach The errors now focus on "numeric" instead of a particular class. Let me know what you think!

alexcwatt commented 1 year ago

@piotrmurach I've made that small change for consistency

alexcwatt commented 11 months ago

@piotrmurach let me know if you want me to do anything else to make this mergeable

piotrmurach commented 11 months ago

@alexcwatt This is on my list, I promise I'm not ignoring you. Please bear with me.