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

Add manual_stop option #53

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

KevSlashNull
Copy link

Describe the change

This adds the manual_stop option (defaulting to false) to multi spinners, so that the top-level spinner will never stop spinning until it is stopped manually.

See examples/multi/multi_manual_stop.rb:

Screencast.from.2024-07-23.01-16-33.mp4

Why are we doing this?

Closes #45

In our specific use case, we’re working on integrating tty-spinner into our development tool GDK to work with Rake and later allow parallel updates. We only want to show spinners for currently running tasks, and not pre-register spinners in case we later want to skip them to reduce visual noise. By gaining manual control over the top-level spinner, we can keep it spinning until all tasks have finished.

Benefits

Present a better user experience in use cases such as our’s or the one detailed in #45.

Drawbacks

Manual control over the top level spinner could be confusing for a developer, so they might end up keeping the spinner running indefinitely, breaking the output until the process exits. Though this can already happen if not all registered are stopped.

Requirements

  • Tests written & passing locally?
  • Code style checked?
  • Rebased with master branch?
  • Documentation updated?
  • Changelog updated?


spinners = TTY::Spinner::Multi.new("[:spinner] Top level spinner", {
manual_stop: true,
})

Choose a reason for hiding this comment

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

Layout/IndentHash: Indent the right brace the same as the first position after the preceding left parenthesis.

require_relative "../../lib/tty-spinner"

spinners = TTY::Spinner::Multi.new("[:spinner] Top level spinner", {
manual_stop: true,

Choose a reason for hiding this comment

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

Layout/IndentHash: Use 2 spaces for indentation in a hash, relative to the first position after the preceding left parenthesis.
Style/TrailingCommaInHashLiteral: Avoid comma after the last item of a hash.


require_relative "../../lib/tty-spinner"

spinners = TTY::Spinner::Multi.new("[:spinner] Top level spinner", {

Choose a reason for hiding this comment

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

Style/BracesAroundHashParameters: Redundant curly braces around a hash parameter.

@@ -49,13 +49,18 @@ class Multi
# clear ouptut when finished
# @option options [Float] :interval
# the interval for auto spinning
# @option options [Boolean] :manual_stop
# the top spinner (if present) doesn’t automatically stop when all

Choose a reason for hiding this comment

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

Style/AsciiComments: Use only ascii symbols in comments.


spinner = spinners.register("three") { |sp| jobs << "three"; sp.success }
spinner.run

Choose a reason for hiding this comment

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

Layout/TrailingWhitespace: Trailing whitespace detected.

it 'keeps the top level spinner spinning' do
spinners = TTY::Spinner::Multi.new("top", output: output, manual_stop: true)
jobs = []

Choose a reason for hiding this comment

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

Layout/TrailingWhitespace: Trailing whitespace detected.


context 'when manual_stop option is set' do
it 'keeps the top level spinner spinning' do
spinners = TTY::Spinner::Multi.new("top", output: output, manual_stop: true)

Choose a reason for hiding this comment

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

Metrics/LineLength: Line is too long. [82/80]

spec/unit/multi/auto_spin_spec.rb Outdated Show resolved Hide resolved
spec/unit/multi/auto_spin_spec.rb Outdated Show resolved Hide resolved
@@ -28,5 +28,27 @@
spinners.auto_spin

expect(jobs).to match_array(%w[one two])
expect(spinners.top_spinner.done?).to be(true)
end

Choose a reason for hiding this comment

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

Layout/TrailingWhitespace: Trailing whitespace detected.

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.

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