Skip to content

chore: improve python-progressbar maintenance path#314

Open
lphuc2250gma wants to merge 1 commit into
wolph:developfrom
lphuc2250gma:maint/20260525120751
Open

chore: improve python-progressbar maintenance path#314
lphuc2250gma wants to merge 1 commit into
wolph:developfrom
lphuc2250gma:maint/20260525120751

Conversation

@lphuc2250gma
Copy link
Copy Markdown

Summary:

  • Add narrow unit tests for edge cases in the utility helpers in progressbar/utils.py (e.g., len_color/no_color/AttributeBuffer behavior with empty strings, ANSI-only input, and mixed escape sequences) and tighten any missing type annotations on small helper functions. Purely additive tests plus minor type hint fixes — no behavior change.
  • Keep the change narrow so it is straightforward to review.

Notes:

  • I kept this scoped to the relevant implementation and tests.

Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request updates the type hints for AttributeDict in progressbar/utils.py to support any value type and introduces a suite of unit tests for color-related utilities and dictionary behavior. The reviewer suggests explicitly importing the utils module in the test file to maintain consistency and recommends expanding the test coverage to include 24-bit TrueColor sequences and byte-based ANSI sequences for length calculations.

Comment thread tests/test_utils.py
],
)
def test_no_color(value, expected) -> None:
assert progressbar.utils.no_color(value) == expected
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

The tests use progressbar.utils, but this submodule is not explicitly imported in the file. Given that progressbar.env is explicitly imported at line 6, it is recommended to also explicitly import progressbar.utils at the top of the file to ensure the tests run correctly and maintain consistency with the existing import style.

Comment thread tests/test_utils.py
(b'\x1b[31m', b''),
('\x1b[1m\x1b[31mtext\x1b[0m', 'text'),
(b'\x1b[1m\x1b[31mtext\x1b[0m', b'text'),
('\x1b[38;5;208mhello world\x1b[0m', 'hello world'),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

Consider adding a test case for 24-bit (TrueColor) ANSI sequences to ensure they are also correctly stripped by no_color.

        ('\x1b[38;5;208mhello world\x1b[0m', 'hello world'),
        ('\x1b[38;2;255;0;0mred\x1b[0m', 'red'),

Comment thread tests/test_utils.py
'value,expected',
[
('', 0),
(b'', 0),
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

medium

While test_len_color includes an empty bytes case, it would be beneficial to add a test case for bytes containing ANSI escape sequences to ensure len_color handles bytes with formatting correctly, similar to the string cases.

Suggested change
(b'', 0),
(b'', 0),
(b'\x1b[31mtext\x1b[0m', 4),

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.

1 participant