Skip to content

tqdm 8 #15

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.

Sign up for GitHub

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

tqdm 8 #15

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion tqdm_8/tqdm/_tqdm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ def format_meter(n, total, elapsed, ncols=None, prefix='',
# Format left/right sides of the bar, and format the bar
# later in the remaining space (avoid breaking display)
l_bar_user, r_bar_user = bar_format.split('{bar}')
l_bar, r_bar = l_bar_user.format(**bar_args), r_bar_user.format(**bar_args)
l_bar, r_bar = l_bar.format(**bar_args), r_bar.format(**bar_args)
Comment on lines 224 to +225
Copy link

@coderabbitai coderabbitai bot May 23, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue

Critical Issue: Custom bar format is being ignored

The code splits the custom bar_format into l_bar_user and r_bar_user on line 224, but then formats the default l_bar and r_bar strings instead. This completely ignores the user's custom format specification.

The correct implementation should format the user-provided strings:

                l_bar_user, r_bar_user = bar_format.split('{bar}')
-               l_bar, r_bar = l_bar.format(**bar_args), r_bar.format(**bar_args)
+               l_bar, r_bar = l_bar_user.format(**bar_args), r_bar_user.format(**bar_args)

This change breaks custom bar formatting functionality where users specify their own left and right bar components.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
l_bar_user, r_bar_user = bar_format.split('{bar}')
l_bar, r_bar = l_bar_user.format(**bar_args), r_bar_user.format(**bar_args)
l_bar, r_bar = l_bar.format(**bar_args), r_bar.format(**bar_args)
l_bar_user, r_bar_user = bar_format.split('{bar}')
l_bar, r_bar = l_bar_user.format(**bar_args), r_bar_user.format(**bar_args)
🤖 Prompt for AI Agents 
In tqdm_8/tqdm/_tqdm.py around lines 224 to 225, the code splits the custom
bar_format into l_bar_user and r_bar_user but mistakenly formats the default
l_bar and r_bar instead, ignoring the user's custom format. To fix this, replace
the formatting calls to apply to l_bar_user and r_bar_user using the bar_args,
ensuring the user's custom bar format is correctly processed and displayed.

Copy link

@blar-app blar-app bot May 23, 2025

Choose a reason for hiding this comment

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

🟠 Warning 🐛 Bug

Use of undefined variables 'l_bar' and 'r_bar' after splitting bar_format into 'l_bar_user' and 'r_bar_user'.

Issue Explanation
  • The code splits 'bar_format' by '{bar}' into 'l_bar_user' and 'r_bar_user'.
  • The original line used 'l_bar_user.format(**bar_args)' and 'r_bar_user.format(**bar_args)'.
  • The change replaced this with 'l_bar.format(**bar_args)' and 'r_bar.format(**bar_args)'.
  • 'l_bar' and 'r_bar' are undefined variables at this point, causing a NameError at runtime.
  • This will break the progress bar formatting logic where partial formatting is done before inserting the progress bar itself.

Reply if you have any questions or let me know if I missed something.

Don't forget to react with a 👍 or 👎 to the comments made by Blar to help us improve.

else:
# Else no progress bar, we can just format and return
return bar_format.format(**bar_args)
Expand Down
6 changes: 1 addition & 5 deletions tqdm_8/tqdm/tests/tests_tqdm.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,13 +214,9 @@ def test_format_meter():
"100KiB [00:13, 7.69KiB/s]"
assert format_meter(100, 1000, 12, ncols=0, rate=7.33) == \
" 10% 100/1000 [00:12<02:02, 7.33it/s]"
# Check that bar_format correctly adapts {bar} size to the rest
assert format_meter(20, 100, 12, ncols=13, rate=8.1,
assert format_meter(20, 100, 12, ncols=30, rate=8.1,
bar_format=r'{l_bar}{bar}|{n_fmt}/{total_fmt}') == \
Comment on lines 216 to 219
Copy link

@blar-app blar-app bot May 23, 2025

Choose a reason for hiding this comment

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

🟠 Warning 🐛 Bug

Removed narrow-width tests for ncols=13 and ncols=14 in test_format_meter remove coverage of compact bar sizing edge cases.

Issue Explanation
  • The test_format_meter test previously included assertions for ncols=13 and ncols=14 narrow bar rendering.
  • These assertions were removed in the current PR, leaving no coverage for narrow-width bar cases.
  • Remaining tests use wider ncols values (e.g., ncols=30), which do not cover potential bugs in fractional/full block calculations for narrow bars.
  • This removal risks regressions in bar sizing logic when displayed in very narrow terminals or limited-width UIs.

Reply if you have any questions or let me know if I missed something.

Don't forget to react with a 👍 or 👎 to the comments made by Blar to help us improve.

" 20%|" + unich(0x258f) + "|20/100"
Comment on lines +217 to 219
Copy link

@coderabbitai coderabbitai bot May 23, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Test change may be masking the underlying issue

This test was modified to use ncols=30 instead of the previous value, likely to accommodate the changed behavior in the format_meter method. However, given the critical issue identified in _tqdm.py where custom bar formats are being ignored, this test change may be validating incorrect behavior.

Consider reverting this test change after fixing the core formatting issue in _tqdm.py. The test should validate that custom bar_format strings work as expected, not adapt to broken behavior.

🏁 Script executed:

#!/bin/bash
# Description: Search for other tests that might be affected by the custom bar_format bug
# Expected: Find tests that use custom bar_format to ensure they still work correctly

rg -A 5 -B 5 "bar_format.*\{.*bar.*\}" --type py

Length of output: 66055

Restore and validate custom bar_format tests after fixing the split-variable bug

The change to use ncols=30 simply masks a typo in _tqdm.py’s split-branch (using undefined l_bar/r_bar instead of l_bar_user/r_bar_user). Once you correct that, revert the test to its original assertions and re-enable the removed variants.

• Fix in tqdm_8/tqdm/_tqdm.py

# before
l_bar_user, r_bar_user = bar_format.split('{bar}')
l_bar, r_bar = l_bar.format(**bar_args), r_bar.format(**bar_args)

# after
l_bar_user, r_bar_user = bar_format.split('{bar}')
l_bar, r_bar = l_bar_user.format(**bar_args), r_bar_user.format(**bar_args)

• Restore original test assertions in tqdm_8/tqdm/tests/tests_tqdm.py around line 217:

- assert format_meter(20, 100, 12, ncols=30, rate=8.1,
-                     bar_format=r'{l_bar}{bar}|{n_fmt}/{total_fmt}') == \
-     " 20%|" + unich(0x258f) + "|20/100"

+ assert format_meter(20, 100, 12, ncols=13, rate=8.1,
+                     bar_format=r'{l_bar}{bar}|{n_fmt}/{total_fmt}') == \
+     " 20%|" + unich(0x258f) + "|20/100"
+ assert format_meter(20, 100, 12, ncols=14, rate=8.1,
+                     bar_format=r'{l_bar}{bar}|{n_fmt}/{total_fmt}') == \
+     " 20%|" + unich(0x258d) + " |20/100"

Also review other bar_format tests in the suite (e.g. the ncols=2/6/7 cases) to ensure they pass once the core bug is fixed.

🤖 Prompt for AI Agents 
In tqdm_8/tqdm/tests/tests_tqdm.py around lines 217 to 219, the test was
modified to use ncols=30, which masks a core bug in _tqdm.py where the variables
l_bar and r_bar are incorrectly used instead of l_bar_user and r_bar_user when
formatting the bar. First, fix the bug in _tqdm.py by replacing l_bar and r_bar
with l_bar_user and r_bar_user after splitting the bar_format string. Then,
revert the test in tests_tqdm.py to its original assertions before the ncols=30
change and re-enable any removed variants. Finally, review other tests using
custom bar_format to ensure they pass with the corrected implementation.

Comment on lines +217 to 219
Copy link

@blar-app blar-app bot May 23, 2025

Choose a reason for hiding this comment

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

🟡 Info 🐛 Bug

Test expectation mismatch for progress bar rendering with ncols=30 at 20% progress

Issue Explanation
  • Test in tests_tqdm.py updated ncols from 13 to 30 for format_meter call.
  • The assertion still expects only a single fractional block (unich(0x258f)) in the progress bar.
  • With ncols=30 and 20% progress, the bar should generate multiple full blocks and possibly a fractional block, not just a single fractional block.
  • This results in the expected output string not matching actual bar rendering output.
  • The test expectation needs to be corrected to align with the wider bar's actual output.

Reply if you have any questions or let me know if I missed something.

Don't forget to react with a 👍 or 👎 to the comments made by Blar to help us improve.

assert format_meter(20, 100, 12, ncols=14, rate=8.1,
bar_format=r'{l_bar}{bar}|{n_fmt}/{total_fmt}') == \
" 20%|" + unich(0x258d) + " |20/100"


def test_si_format():
Expand Down