piotrmurach / tty-progressbar

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

Option `no_width` should be documented in README.md #43

Closed hyfen closed 3 years ago

hyfen commented 4 years ago

In certain situations, we might not know total in advance but we might still want to display a progress bar that shows the current count and elapsed count.

I was going to create an issue and start working on PR for this but then I noticed that there's an option called no_width that addresses this already. Is there a reason it's not in the README? I could take a stab at documenting it. I also noticed that some of the formatters rely on having total available to them.

piotrmurach commented 4 years ago

Hi Andrew,

Thanks for using tty-progressbar.

The no_width option doesn't do anything. It's there as a remnant from early code refactorings and probably should've been removed long time ago. In the past, I thought about having 'indeterministic' progression but decided against the idea and instead created tty-spinner. The spinner allows you to display arbitrary info including current count etc... You could split the processing into spinner for the indeterministic part and replace it later with progression once the total is known.

As for having indeterministic progression in tty-progressbar, I'm seeing few issues with it. Mostly, how would the progression behave? Let's say you set count to some value and this brings the progression halfway. What if later the current position is only a small portion of the total, do you backtrack progression or freeze it for the time it needs to 'catchup'? From the user perspective, it's important to get clear behaviour which may not be a trivial problem to solve. I could argue that if you don't know the initial total then you can use any 'reasonable' estimation like double the current count until you know the precise value.

Having said all that, if you have an idea and want to discuss or submit PR I'm more than happy to review and provide feedback.

hyfen commented 4 years ago

Thanks for the explanation. Adding the no_width: true option was enough to prevent a crash on advance() because of this guard. It's working fine as long as I change the format string to not include fields that require total.

Some context: I'm writing a CLI tool that runs ETL jobs where the number of items might not be known in advance. I'd love to use the same progress bar class for both scenarios. As long as you don't remove the line of code I linked to above, things are working fine. :)

Previous to tty-progressbar, I was using ruby-progressbar which has an option to for unknown progress. It's basically a vertical spinner as well as "??" for fields like ETA or the total. Details here.. There's no concept of progression or estimation of remaining time; there's simply an incrementing count as well as the tracking of total time and mean rate.

Do you see this sort of feature belonging in this gem?

piotrmurach commented 4 years ago

I'm convinced, let's add this feature!

Thanks for providing the context and I don't want to break things unnecessarily so I won't remove the option. However, I'm not sure that no_width is an appropriate name for 'unknown' progress. If I recall correctly, what I was trying to do here is provide an alternative to setting specific width. This is all to say that I would welcome some new option name here or make the total configuration recognise value like :unknown etc...

I like the idea of animation for indeterminate progress, how about having == move left and right? For formatters like ETA I would display --:-- rather than ?? as a lot of terminal tools work this way and that would be familiar. To direct the effort what I think needs doing are the following(in no particular order):

  1. Animation/behaviour for indeterminate progress with tests
  2. Changes to formatters to account for no total with tests(ideally test behaviour for all formatters)
  3. Example in the examples
  4. Readme entry

What do you reckon?

hyfen commented 4 years ago

Sounds good! I can take a stab at this.

What do you think of the idea of using total: nil to trigger indeterminate behaviour instead of passing an unknown: true?

piotrmurach commented 4 years ago

I'm not so sure about total: nil. In general, I see nil as an anti-pattern for configuration options as nil can stand for the value of nil or lack of any value. So if total becomes nil, does that mean that it was intentional or is it possible there is a bug that caused total to be nil? What if someone fails to specify total option altogether? As for gem maintenance and code readability, the logic that deals with nil starts to show very little meaning and makes nil errors harder to identify. I kind of lean towards :indeterminate as actual configuration option? The :unknown seems too vague for this purpose. We could then have indeterminate? check on the progress bar instance itself. I think this would lead to more transparent code in formatters as well as in the client code.

piotrmurach commented 4 years ago

@hyfen Any progress?

piotrmurach commented 4 years ago

@hyfen Good news, I've added support for indeterminate progress! I went with the total: nil as using indeterminate: true made for more cumbersome implementation and user exeperience. Please see indeterminate example of how this works. Looking for your feedback!

hyfen commented 4 years ago

Hi Piotr, this looks great and I'm excited to try it out! I'm not able to be at my computer much this month but I'll try to have a look in the next week.

piotrmurach commented 4 years ago

@hyfen I'm planning to make a release this coming weekend and would love to hear your opinion before I publish a new version. Do you think you could find a minute to check this out?