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

Fix: Add display() call to close() in rich module #1395

Merged
merged 1 commit into from May 2, 2024

Conversation

glensc
Copy link
Contributor

@glensc glensc commented Nov 21, 2022

See how I've come to this conclusion. This note and notes before it:

Testcase:

from tqdm.rich import trange, tqdm
for i in trange(10):
    print(f"i={i}")
print("finished")

This snippet, when run, must reach 100%. without the patch it doesn't even get past 0.

The display() calls in std.close are never reached because the "gui mode check" exits the function:

Fixes:

tqdm/rich.py Outdated

# Add call display to print 100%
# https://github.com/tqdm/tqdm/issues/1306#issuecomment-1322762835
self.display()
Copy link
Contributor Author

@glensc glensc Nov 21, 2022

Choose a reason for hiding this comment

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

Suggested change
self.display()
if self.leave:
self.display()

should it be with self.leave check like gui.py?

@glensc glensc changed the title Fix: Add display to close in rich module Fix: Add display() call to close() in rich module Nov 21, 2022
@glensc
Copy link
Contributor Author

glensc commented Nov 23, 2022

@casperdcl do you need some help maintaining this project?

@codecov
Copy link

codecov bot commented Nov 26, 2022

Codecov Report

Merging #1395 (6791e8c) into master (6791e8c) will not change coverage.
The diff coverage is n/a.

❗ Current head 6791e8c differs from pull request most recent head 4d75eb0. Consider uploading reports for the commit 4d75eb0 to get more accurate results

@@           Coverage Diff           @@
##           master    #1395   +/-   ##
=======================================
  Coverage   88.94%   88.94%           
=======================================
  Files          26       26           
  Lines        1764     1764           
  Branches      344      344           
=======================================
  Hits         1569     1569           
  Misses        145      145           
  Partials       50       50           

@glensc
Copy link
Contributor Author

glensc commented Dec 11, 2022

Monkey patching in my project:

@glensc
Copy link
Contributor Author

glensc commented Aug 16, 2023

@casperdcl my monkey patching no issues so far:

@glensc glensc force-pushed the patch-1 branch 2 times, most recently from a4eaa99 to fe68772 Compare September 11, 2023 20:58
@sybrenjansen
Copy link

I know this PR is a bit old, but I was having the issue as well and decided to check if there was any fix. This PR actually didn't fix it for me, at least not completely.

The display call is needed, but the display implementation also needs to be updated to:

    def display(self, *_, **__):
        if not hasattr(self, '_prog'):
            return
        self._prog.update(self._task_id, completed=self.n, description=self.desc, refresh=True)

Mind the refresh=True at the end.

The default (https://github.com/Textualize/rich/blob/master/rich/progress.py#L1421) is set to False, but this will actually force a refresh and made my issues go away.

@glensc
Copy link
Contributor Author

glensc commented Jan 5, 2024

it has been rebased to the last release: v4.66.1, so not old.

@glensc
Copy link
Contributor Author

glensc commented Jan 5, 2024

@sybrenjansen

🚧 Never link to branches, share a permalink 🚧

NOTE: edit your post and fix it with a permalink!

@glensc
Copy link
Contributor Author

glensc commented Jan 6, 2024

@sybrenjansen what's your reproducer? perhaps it's something else (create new issue with it?), as this fixes for me and other users in #1306

@casperdcl casperdcl changed the base branch from master to devel May 2, 2024 22:05
@casperdcl casperdcl merged commit 7c8753f into tqdm:devel May 2, 2024
12 of 13 checks passed
@casperdcl
Copy link
Sponsor Member

argh sorry for the epic delay... will release in tqdm>=4.66.4 shortly

@casperdcl casperdcl added submodule ⊂ Periphery/subclasses to-merge ↰ Imminent p2-bug-warning ⚠ Visual output bad c1-quick 🕐 Complexity low labels May 2, 2024
@casperdcl casperdcl added this to Done in Casper May 2, 2024
@casperdcl casperdcl mentioned this pull request May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c1-quick 🕐 Complexity low p2-bug-warning ⚠ Visual output bad submodule ⊂ Periphery/subclasses to-merge ↰ Imminent
Projects
Casper
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants