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

progress: Improve the Progress Plugin #622

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

RobLoach
Copy link
Contributor

@RobLoach RobLoach commented Oct 3, 2024

This change improves the progress plugin by...

  • Allows reseting the progress bar to allow running another progress bar after the first one is done
  • Replaces the message whitespace so that there's no overlap in messages
  • Scopes the variables within the function
  • Renames the global variable so that it's namespaced a bit

Here's a good way to test it...

progress 0 "Doing something cool!" && progress 40 "Hello?" && progress 60 "Almost done" && progress 100 "Close" && delay

plugins/progress/progress.plugin.sh Outdated Show resolved Hide resolved
plugins/progress/progress.plugin.sh Outdated Show resolved Hide resolved
plugins/progress/progress.plugin.sh Outdated Show resolved Hide resolved
plugins/progress/progress.plugin.sh Outdated Show resolved Hide resolved
plugins/progress/progress.plugin.sh Outdated Show resolved Hide resolved
delay
printf "%s\n" "$clear_line"
value=0
fi;
Copy link
Contributor

Choose a reason for hiding this comment

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

The message Done! seems to have been dropped. Is this an intentional behavioral change? What is the rationale?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's kind of clear when it's done, because the application continues?

Copy link
Contributor

@akinomyoga akinomyoga Dec 5, 2024

Choose a reason for hiding this comment

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

That explains that the new behavior works fine, but it is the same for the previous behavior. Rather, the previous behavior was even more clear about that. Also, does that "?" mean you don't remember why you did it? So, can I think it was not the intentional behavioral change? I avoid breaking changes unless there is a clear reason. Could you recover the original behavior?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When comparing other progress bar approaches, like that in apt, I found that it didn't present a Done! For each progress bar that was displayed. Tried to mimic that behavior.

The removal was intentional, but I'm happy to restore it.

Copy link
Contributor

@akinomyoga akinomyoga left a comment

Choose a reason for hiding this comment

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

Another thing that this PR changes without explanation is the accelerating growth of the progress bar. Now it is linear. What is the reason for this change? Did you remove that knowing the original intent? Or did you just remove it because you don't understand it?

* Update plugins/progress/progress.plugin.sh
* Update to use clear lines
* Use . instead of -

Co-authored-by: Koichi Murase <[email protected]>
@RobLoach
Copy link
Contributor Author

Thanks for following up on this. Completely forgot about it.

@akinomyoga
Copy link
Contributor

Thanks for checking it. If you are fine with the new changes I pushed, I'll squash them and merge the PR.

@RobLoach
Copy link
Contributor Author

Tested it out, and it works really well. Thanks so much 🚀

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.

2 participants