-
Notifications
You must be signed in to change notification settings - Fork 271
Add finish_style setting #728
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
Conversation
| @@ -0,0 +1,24 @@ | |||
| use std::thread; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please drop the extra example, we already have too many.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other examples don't illustrate the feature. Could you please, provide the alternative?
| pub(crate) draw_target: ProgressDrawTarget, | ||
| pub(crate) on_finish: ProgressFinish, | ||
| pub(crate) style: ProgressStyle, | ||
| pub(crate) finish_style: Option<ProgressStyle>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of adding more state, suggest adding a new variant to ProgressFinish instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your way it will be way more complicated than it should to be.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see any reason why both styles should be stored in different places.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Semantically, finish style has more to do with ProgressFinish struct than BarState.
The whole job of ProgressFinish is to have all the information about what to do after progress is finished. I believe that finish style is part of that and not part of ProgressState
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that this would complicate code which is already quite complicated.
If you still insist, I'd like to ask for a patch proposition.
|
Since you're not taking any feedback, I'll just close this. |
|
@djc I expressed my concerns and asked for help. I don't know how it could be considered by |
Resolves #355