search

piotrmurach / tty-spinner

A terminal spinner for tasks that have non-deterministic time frame.
https://ttytoolkit.org
MIT License
427 stars 28 forks source link

Feature Request: Allow top_spinner to continue even if the tasks below is in error. #45

Open dipanm opened 3 years ago

dipanm commented 3 years ago

I am using a TTY::Spinner::Multi

It has multiple child spinners. However, even if a single child fails, the top-level spinner actually stops. The child spinners are async with auto_spin.

As per documentation: Finally, you need to stop each spinner manually, in our case we mark the second spinner as failure which in turn will stop the top-level multi spinner automatically and mark it as a failure

This is limiting in my use case. I use fields at top level spinner to indicate some aggregate score. Also, in our context, if one of the spinners (and corresponding tasks) fails, a new task is added with the corresponding spinner. So a given spinner failure doesn't imply failure of the top-level spinner and it should not stop. We can explicitly stop the top-level spinner when the full task is done.

In general, there can be many scenarios, where a top-level spinner needs to continue till at least one child is running or maybe even after all children are running. The current behavior could be one of the options or default behavior but should not be a forced choice.

piotrmurach commented 3 years ago

That sounds reasonable to me. Do you have a suggestion from the API standpoint how this could be resolved? Any code snippets would be welcome.

frsantos commented 2 years ago

I too would need stopping the top spinner manually, since I can add spinners to the multi after the previous ones have succeeded or failed.

I think that adding a flag to the stop/success/error would be cumbersome, as we don't know the real state of the multi until we manually stop it. A failed spinner could be retried with a new spinner, as @dipanm said, so the state of the multi could end as success even though one of the spinner has failed.

Probably the best approach could be to allow detaching the state of the multi, so we can set it manually. A flag on the multi constructor could be of use. link_states: true by default, for example.

piotrmurach commented 2 years ago

I'd suggest we tackle this by introducing a new option named manual_stop for the multi-spinner.

spinners = TTY::Spinner::Multi.new(":spinner", manual_stop: true)

This would disable any automatic stopping of the top-level spinner when any of the child spinners errors or all of them succeed. I'd keep the current behaviour as is to keep it compatible. My preference is when a library out of the box does what is the most common use case and similarly allows to disable it.

@frsantos I believe the idea above matches your suggestion. However, I'd be reluctant to use link_states as the flag due to its ambiguous meaning. The multi-spinner state is somewhat complex and any potential user could be confused about such flag's purpose. Fundamentally, what we're trying to do here is disable automating stopping of the multi-spinner. Hence I think that manual_stop or auto_stop would convey the meaning more intuitively. Wouldn't you agree?

frsantos commented 2 years ago

That would work.

However, I am no longer working on the project where I needed the feature, and so I'm no longer using the gem.