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: make message rendering work 100% (alt fix for #164) #165

Merged
merged 1 commit into from
Apr 22, 2021

Conversation

jedib0t
Copy link
Owner

@jedib0t jedib0t commented Apr 22, 2021

Proposed Changes

The change in #162 missed an instance where tracker.Message was being accessed directly. @virtuald found the issue and proposed a fix via #164, but a better way to fix the root-cause is to read the tracker message ONLY ONCE per render. With the additional benefit of leaving the user's message untouched in the tracker (no padding/snipping). Reading it multiple times in a single render always has the change of causing issues down the road.

This still leaves the sorting logic unfixed, but that will probably need deeper plumbing.

@coveralls
Copy link

Pull Request Test Coverage Report for Build 775103362

  • 21 of 21 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 774792570: 0.0%
Covered Lines: 2942
Relevant Lines: 2942

💛 - Coveralls

@virtuald
Copy link
Contributor

virtuald commented Apr 22, 2021

The only concern I would have is that it's potentially recomputing a lot of information each time it renders (with pad/snip), but presumably that would only be an issue for many thousands of progress bars so we're probably fine? And maybe it's already doing that so it's not a big deal.

This seems like it would be a good alternative to my PR.

@jedib0t jedib0t merged commit 6d0f408 into main Apr 22, 2021
@jedib0t
Copy link
Owner Author

jedib0t commented Apr 22, 2021

The only concern I would have is that it's potentially recomputing a lot of information each time it renders (with pad/snip), but presumably that would only be an issue for many thousands of progress bars so we're probably fine? And maybe it's already doing that so it's not a big deal.

This seems like it would be a good alternative to my PR.

Yeah this is going to hit the performance a bit. We can optimize for it in a new change.

@jedib0t jedib0t deleted the progress-update-message branch April 22, 2021 18:15
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.

3 participants