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

Unwrap ProgressFinish #275

Merged
merged 3 commits into from
Apr 16, 2021
Merged

Unwrap ProgressFinish #275

merged 3 commits into from
Apr 16, 2021

Conversation

mibac138
Copy link
Contributor

This changes default behavior - now a progress bar without an explicitly set on_finish will call finish, when previously it would call finish_and_clear (src/style.rs:103). Also, on_finish won't be reset every time finish_with_style is called.

@djc
Copy link
Collaborator

djc commented Apr 16, 2021

Thanks for the PR -- can you explain why you think these changes represent an improvement? I like that we no longer need Option from a code perspective, but I'm unsure the behavorial changes are actually desirable:

  • Change the default finish behavior
  • Keep on_finish the same throughout resetting finish_with_style

And also, would you mind going over your other open PRs to see if they are still relevant/useful?

Preserves the historical behavior
@mibac138
Copy link
Contributor Author

I'm working on #192, probably will update today, and #132 will be obsolete once I finish another PR, no eta on it though. #256 I have currently postponed and will do some refactoring before returning to it.

  • I agree about changing the default finish behavior but didn't want to change ProgressFinish::default without feedback. Changed it now to keep the same behavior.
  • I find it surprising that on_finish is reset after finish_with_style. I think this is the expected behavior from an end user perspective.

src/style.rs Outdated
@@ -15,14 +15,14 @@ use unicode_segmentation::UnicodeSegmentation;
pub enum ProgressFinish {
/// Finishes the progress bar and leaves the current message.
/// Same behavior as calling [`ProgressBar::finish()`].
Default,
Finish,
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it would be clearer if we call this AndLeave, what do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good, it's more consistent with the rest, changed it

@djc djc merged commit d5f241a into console-rs:main Apr 16, 2021
@mibac138 mibac138 deleted the on-finish branch April 16, 2021 13:36
aj-bagwell pushed a commit to aj-bagwell/indicatif that referenced this pull request May 20, 2021
* Unwrap ProgressFinish

* Change ProgressFinish::default to _::AndClear

Preserves the historical behavior

* Rename `ProgressFinish::Finish` to `_::AndLeave`
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.

None yet

2 participants