-
Notifications
You must be signed in to change notification settings - Fork 114
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: add incremental failure mode #160
Conversation
0ba7af6
to
e9327b9
Compare
I feel that we are stepping into over engineering a solve here for a problem that may never show up. I use this library in a private project where the Message is changed as part of the execution based on where the activity is at. And used by more than a few hundred folks on a daily basis. I understand this is a personal anecdote and may not reflect your observations. The only place the message is used is to render the text on the screen, and this gets updated more than a few times a second with the default setting so this should not cause any noticeable issues to warrant all these changes. You should be able to change the Message including coloring it manually with something along the lines of: tracker.Message = text.Red.Sprint("oops this thing failed!") If you do see issues with changing the Message value, please share a code sample and we can go from there. |
The race detector really doesn't like it when you change tracker.Message during execution, see #161 for a demonstration. |
I'm sure there will be a race condition here since the value is being read and written at the same time, but is it affecting the functionality adversely? Even if the message is not updated on screen immediately, it will be in the next render pass. Regardless, I feel something like |
In any case, this PR is not proposing to change tracker.Message (which because of semver would require an upgrade to v7.x?). This is merely making it possible to mark a progress bar as an error before it completes, which seems like a pretty reasonable thing to do. The only part that's a particularly complex addition would be the demo -- but I could remove that if you want. |
Yes that would require a major version bump. 😢 As for this PR: I'm okay with the change. But, can you please revert the demo changes, and add a unit-test for the new function in tracker_test.go? |
- Changes SetValue behavior but I think that's fine
e9327b9
to
bc9a669
Compare
Done, thanks! |
Thought about it, looks like changing the message is likely to be difficult, so I'm just going to highlight it instead. Changes SetValue behavior but I think that's fine since it's a relatively new change?
Fixes #159