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: add incremental failure mode #160

Merged
merged 1 commit into from
Apr 21, 2021

Conversation

virtuald
Copy link
Contributor

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

@virtuald virtuald force-pushed the incremental-failure branch 2 times, most recently from 0ba7af6 to e9327b9 Compare April 21, 2021 04:09
@coveralls
Copy link

coveralls commented Apr 21, 2021

Coverage Status

Coverage decreased (-0.1%) to 99.864% when pulling bc9a669 on virtuald:incremental-failure into 4e6e466 on jedib0t:master.

@jedib0t
Copy link
Owner

jedib0t commented Apr 21, 2021

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.

@virtuald
Copy link
Contributor Author

The race detector really doesn't like it when you change tracker.Message during execution, see #161 for a demonstration.

@jedib0t
Copy link
Owner

jedib0t commented Apr 21, 2021

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 tracker.UpdateMessage() would be a cleaner approach to minimize races (not remove it altogether). Or making Message private and introducing a tracker.SetMessage() to remove races altogether.

@virtuald
Copy link
Contributor Author

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.

@jedib0t
Copy link
Owner

jedib0t commented Apr 21, 2021

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
@virtuald
Copy link
Contributor Author

Done, thanks!

@jedib0t jedib0t merged commit bc715c2 into jedib0t:master Apr 21, 2021
@virtuald virtuald deleted the incremental-failure branch April 22, 2021 00:09
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.

progress: would like to indicate error before completion
3 participants