Quality audit PR 3/6: bar.py internals, decompositions, start() race fix#321
Merged
Conversation
ProgressBar.start() called super().start() (which sets _started=True via ProgressBarMixinBase.start) before building default_widgets(), so a concurrent reader — e.g. MultiBar's render thread reading bar.started() then asserting bar.widgets in _label_bar — could observe started() True with an empty widget list and crash. Move the cooperative super().start() dispatch to run after all widget/prefix/suffix/poll/gate/max_value setup completes. The 0% draw still happens at the same point (stream redirect and console mode are set up in super().start(), before the draw). Add a deterministic regression test capturing the widget list at the exact _started flip.
data() secretly reset _last_update_time/_last_update_timer on every call, so a getter mutated timing state. Extract the reset into a private _mark_update() and call it from _update_parents() (the redraw path), where the gate calibration in _draw_and_recalibrate already expects the timer to be refreshed during the draw. data() is now a pure read with no timing side effects; the public last_update_time and gate cadence are unchanged because the stamp still happens once per redraw at the same point. Add a regression test (deterministic advancing clock) asserting two consecutive data() calls leave the timing fields untouched.
_load_widgets() called importlib.import_module on every invocation — once per full-bar render site (_format_widgets, default_widgets, prefix/suffix init). The module is immutable after first import, so wrap the helper in functools.cache: importlib resolves progressbar.widgets once on first use and reuses the object thereafter. Name/signature unchanged; the fast path still never calls _load_widgets() so it stays widgets-free (lazy-import tests remain green).
The '_format_fast_line ... Wired in a later task' comment described work that never happened and read as dead indirection. It is in fact a tested, supported extension point (test_fast_format_line_uses_native_hook monkeypatches it, and _format_line prefers it over the pure-Python formatter). Keep the hook and the fallback; replace the misleading comment with an accurate description of the optional native/custom formatter hook. Also annotate default_widgets(self) -> list[typing.Any].
The constructor mixed alias deprecation, widget copying, poll-interval setup, and variable seeding — enough to warrant a 'sourcery skip: low-code-quality' marker. Extract four private, single-purpose helpers and reduce __init__ to orchestration: - _apply_deprecated_aliases(): resolve maxval/poll DeprecationWarnings, returns the updated (max_value, poll_interval) - _copy_widgets(): deepcopy the copy-safe widgets into a fresh list - _setup_poll_intervals(): timedelta->seconds conversion and clamping - _seed_variables(): variables dict + VariableMixin widget-name scan Order of operations is unchanged, so construction behavior is identical. __init__'s signature is byte-identical (only the sourcery comment is dropped, no longer needed).
If an earlier test imports a module while freezegun is active, module constants like widgets.MAX_DATE are Fake* instances for the rest of the process; describe instances by their first non-freezegun MRO class so the snapshot no longer depends on test collection order.
Extract the input resolution, widget selection and copy loop out of the 125-line main() god function into three private single-purpose helpers: - _resolve_inputs(args, parser): stdin/file resolution + total-size detection - _build_widgets(args, filesize_available): widget-set selection - _transfer(bar, input_paths, output_stream, args, stack): the copy loop main() is now pure orchestration and passes flake8 C901 without the '# noqa: C901' suppression, which is removed. Behavior-preserving: CLI output is byte-identical and the pty-based command tests pass unchanged.
from_env() mixed three unrelated detection strategies inline. Extract each into a focused classmethod, leaving from_env() as a precedence-ordered dispatcher: - _from_jupyter(): interactive-kernel true-color shortcut - _from_windows(): Windows console-mode probe - _from_term_variables(variables): terminal env-var depth scan Behavior-preserving: the JUPYTER -> Windows -> env-scan precedence and the subtle scan semantics (first truecolor/24bit wins via break; otherwise the highest depth wins via max(); a generic truthy FORCE_COLOR=1 returns full color) are unchanged. The 'elif os.name == "nt"' dispatch line keeps the existing coverage exclusion; _from_windows() carries the '# pragma: no cover' that previously sat on the Windows block (unreachable on non-Windows CI).
ETA.__call__ and SmoothingETA.__call__ shared an identical value/elapsed default-resolution preamble. Extract it into ETA._resolve_value_elapsed() and call it from both; the shared helper's branches are covered by the ETA paths so SmoothingETA no longer needs its two '# pragma: no branch'. Also replace the TypeError-as-control-flow around _calculate_eta with an explicit 'progress.max_value is base.UnknownLength' check. The only source of that TypeError was subtracting the value from the UnknownLength sentinel inside _calculate_eta's 'if elapsed:' branch, so the explicit check is guarded by 'elapsed' too: this keeps the elapsed==0 case returning 0 (format_zero) instead of N/A, preserving byte-identical output. Nothing else in the ETA math raises TypeError, so the try/except is removed entirely rather than kept as a defensive handler. Behavior-preserving: full suite + render goldens unchanged, widgets.py at 100% branch coverage.
…preamble The identical left/right border resolution (resolve callables, subtract their visible length from width) was copy-pasted across five bar __call__ methods. Extract it into Bar._render_borders(progress, data, width) -> (left, right, remaining_width) and call it from Bar, BouncingBar, MultiRangeBar and JobStatusBar (all Bar subclasses). GranularBar descends from AutoWidthWidgetBase (not Bar) so it can't reach the helper; rather than hoisting border concerns onto a shared width-widget base (a responsibility smell affecting widgets that have no borders) or changing any MRO, its copy is kept with an explanatory comment. Behavior-preserving: widget render output byte-identical, full suite + render goldens pass, widgets.py at 100% branch coverage.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors several modules in the progressbar library to improve code quality, readability, and structure. Key changes include extracting helper methods in __main__.py, bar.py, env.py, and widgets.py, caching lazy widget imports, and separating timing side effects from the data() state snapshot. Additionally, it addresses a race condition in ProgressBar.start() where the started flag was set before widgets were populated. The review feedback highlights two potential TypeError bugs: one in widgets.py when progress.max_value is None for indeterminate bars, and another in test_api_surface.py if cls.__module__ is not a string.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
There was a problem hiding this comment.
Pull request overview
This PR continues the quality audit series by refactoring internal progress bar code for clarity/perf, tightening a few behavioral contracts with regression tests, and addressing a concurrency race in ProgressBar.start() visibility.
Changes:
- Fixes a
start()ordering race so_startedbecomes observable only after widgets are populated (MultiBar render-thread safety). - Refactors internals for maintainability/performance (e.g., cached lazy widget import, decomposed init/start logic, shared helpers in ETA/bar widgets).
- Adds/adjusts regression tests to lock in purity/order-independence guarantees (
data()snapshot purity; API surface snapshot stability under freezegun).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/test_progressbar.py | Adds a regression test ensuring data() is a pure snapshot (no timing side effects). |
| tests/test_multibar.py | Adds a regression test guarding the _started/widgets ordering race relevant to MultiBar. |
| tests/test_api_surface.py | Makes API surface snapshot stable even when earlier tests import modules under freezegun. |
| progressbar/widgets.py | Refactors ETA value/elapsed resolution and deduplicates bar border rendering logic. |
| progressbar/fast.py | Documents the native formatter hook and tightens typing on default_widgets. |
| progressbar/env.py | Decomposes ColorSupport.from_env into precedence-preserving helpers. |
| progressbar/bar.py | Caches _load_widgets, decomposes __init__, moves timing stamps out of data() into _mark_update, and reorders start() for race safety. |
| progressbar/main.py | Refactors CLI main() into helpers for input resolution, widget selection, and transfer loop. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5927aae5dd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The _transfer extraction moved it outside, so BrokenPipeError from a piped stderr could crash shutdown (flagged by review on #321); restore the historical placement.
A MultiBar render thread that sees started() True calls update(force=True), and update() re-enters start() while start_time is None -- double-running the stream-capturing path. Populate start_time/last_update_time before the cooperative start() dispatch flips _started (flagged by review on #321); flip-observation test extended to assert it.
The explicit UnknownLength check replaced a broad try/except TypeError; None is also a legitimate indeterminate max_value and must take the N/A path (flagged by review on #321).
This was referenced Jul 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Third of six audit PRs (PR 1: #319, PR 2: #320). Behavior-preserving internal restructuring plus one real concurrency bug fix.
Bug fix
start()ordering race (d7cb0c1):_startedflipped true beforeself.widgetswas populated, so a concurrent reader — MultiBar's render thread callsbar.started()then assertsbar.widgets— could crash on the empty-widgets window (this exact crash appeared as a reproducible py312 CI coverage failure during PR 1). Widget/state setup now completes before_startedbecomes observable; deterministic regression test captures the widget list at the exact flip.Decompositions (no behavior change; suite + render goldens + byte-identical CLI output as oracle)
ProgressBar.__init__(~100 lines) → four single-purpose private helpers; the self-deprecating# sourcery skip: low-code-qualityis gone (842b6d4).__main__.main()(125 lines,noqa: C901) →_resolve_inputs/_build_widgets/_transfer; passes the complexity check naturally now (ca6553f).ColorSupport.from_env→ per-source helpers with the subtle precedence preserved (3aaf461)._resolve_value_elapsed; the UnknownLength path is now an explicit check instead of a caught TypeError (guarded to keepelapsed == 0renderingformat_zero, byte-identical) (59b3cb2)._render_bordershelper replaces the border preamble copy-pasted across 4 subclasses (GranularBar keeps its copy — different MRO branch — with a pointer comment) (5927aae).Internals
data()is now a pure snapshot; the timer reset moved to_mark_update()on the draw path (gate calibration cadence verified unchanged by the fastpath parity tests) (04d57e8)._load_widgets()cached viafunctools.cache— full-bar render sites no longer pay animport_modulelookup per call; the fast path still never imports widgets (test-enforced) (8c82288)._format_fast_linedocumented as the optional native-formatter extension point (007f5f1).960657e).Verification
569 passed / 100.00% branch coverage (3.14); CI-parity (TERM=dumb) green on 3.10/3.12; ruff + pyright clean; perf budget passes; multibar suite 3× consecutive green; CLI output byte-compared identical.