-
-
Notifications
You must be signed in to change notification settings - Fork 24
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
Fix setting the current progress for an indeterminate bar #56
Fix setting the current progress for an indeterminate bar #56
Conversation
3b74608
to
9a60a52
Compare
9a60a52
to
d0dca1d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi Alex 👋
Thank you for using the tty-progressbar and submitting this PR.
I left a comment about the parameter type check. Let me know what you think. The current support for indeterminate progress change is, of course, desirable.
d0dca1d
to
d02410f
Compare
d02410f
to
c6f61b2
Compare
@piotrmurach The errors now focus on "numeric" instead of a particular class. Let me know what you think! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for making changes so quickly. There is a tiny issue to address for consistency. That should be all.
c6f61b2
to
110530b
Compare
@piotrmurach I've made that small change for consistency |
@piotrmurach let me know if you want me to do anything else to make this mergeable |
@alexcwatt This is on my list, I promise I'm not ignoring you. Please bear with me. |
110530b
to
75849ff
Compare
@alexcwatt Released |
Describe the change
I noticed that
#current=
failed for indeterminate progress bars. The primary test I added would error like this: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 thatcurrent = nil
was already raisingArgumentError
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 acurrent
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
master
branch?