Clean up and enhance progress implementation #646
Conversation
bmr-cymru
added
enhancement
WalkthroughAdds a ProgressBase with lifecycle hooks and width-calculation, factors terminal colour init into _init_colors, renames QuietProgress → NullProgress, adds no_clear to Progress, broadens curses init error handling, and updates exports and tests to reflect the new API. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
1 similar comment
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_progress.py (1)
91-130: Remove unused variable assignments.The
bpvariables are assigned but never used since theValueErroris raised during__init__. The assignment is unnecessary and triggers static analysis warnings (F841).Apply this diff to remove the unused assignments:
with self.assertRaisesRegex(ValueError, r"self\.FIXED must be"): - bp = BadProgress() + BadProgress() def test_bad_child_no_header(self): class BadProgress(ProgressBase): FIXED = 1 def __init__(self): self.width = self._calculate_width(width=20) def _do_start(self, _total: int): pass def _do_progress(self, _done: int, _message: str): pass def _do_end(self, _message: str): pass with self.assertRaisesRegex(ValueError, r"self\.header must be"): - bp = BadProgress() + BadProgress()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snapm/_progress.py(16 hunks)snapm/_snapm.py(2 hunks)tests/test_progress.py(9 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
🪛 Ruff (0.14.7)
tests/test_progress.py
110-110: Local variable bp is assigned to but never used
Remove assignment to unused variable bp
(F841)
129-129: Local variable bp is assigned to but never used
Remove assignment to unused variable bp
(F841)
snapm/_progress.py
158-158: Do not catch blind exception: BaseException
(BLE001)
261-264: Avoid specifying long messages outside the exception class
(TRY003)
267-270: Avoid specifying long messages outside the exception class
(TRY003)
273-273: Avoid specifying long messages outside the exception class
(TRY003)
297-297: Avoid specifying long messages outside the exception class
(TRY003)
325-325: Avoid specifying long messages outside the exception class
(TRY003)
328-328: Avoid specifying long messages outside the exception class
(TRY003)
331-331: Avoid specifying long messages outside the exception class
(TRY003)
443-443: Avoid specifying long messages outside the exception class
(TRY003)
646-646: Unused method argument: done
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (34)
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (17)
snapm/_snapm.py (1)
25-31: LGTM!The rename from
QuietProgresstoNullProgressis applied consistently in both the import and the__all__export list, aligning with the PR objective to rename whilst preserving the higher-levelquiet=Trueinterface.Also applies to: 2271-2271
tests/test_progress.py (6)
62-74: LGTM!The test fix correctly validates BLACK (index 0 in
_ANSI_COLORS) rather than GREEN, matching the mockedtparmreturn value of\x1b[30m. This aligns with PR objective #643.
207-241: LGTM!The
test_lifecycle_no_cleartest properly validates that whenno_clear=True, the progress bar header and final message are preserved in the output (verifying 2 lines). This aligns with PR objective #639.
265-279: LGTM!The new tests for
end_before_start_raisesandprogress_after_end_raisesproperly validate the state machine enforcement, ensuring thatprogress()andend()cannot be called beforestart(), and thatprogress()cannot be called afterend(). This aligns with PR objective #635.
321-335: LGTM!The new lifecycle validation tests for
SimpleProgressensure consistent state machine enforcement across all progress implementations.
350-399: LGTM!The test class is properly renamed to
TestNullProgresswith all references updated. The new lifecycle validation tests ensureNullProgressalso enforces the same state machine semantics as other progress implementations.
402-406: LGTM!Factory test correctly verifies that
quiet=Truereturns aNullProgressinstance.snapm/_progress.py (10)
17-24: LGTM!Good extraction of magic numbers into named module-level constants. This improves maintainability and aligns with PR objective #637 for factoring width calculation via class variables.
129-129: LGTM!The rename to
_ANSI_COLORSfollows Python naming conventions, and the use ofenumerate()is more idiomatic thanzip(range(...), ...). This also correctly fixes the ANSI foreground colour initialisation (PR objective #642).Also applies to: 174-196
150-159: Justified broad exception catch.The
BaseExceptioncatch is intentional as documented in the comment and aligns with PR objective #644. The curses module's error class exhibits unusual behaviour that requires this approach. The static analysis warning (BLE001) can be safely ignored here.
221-287: LGTM!The
ProgressBaseABC correctly implements the template method pattern with centralised validation. TheFIXEDclass variable and_calculate_width()method factor out common width calculation logic (PR objective #637), and instance attributes now have type hints (PR objective #640).
289-378: LGTM!The lifecycle methods (
start,progress,end) correctly implement state machine enforcement via_check_in_progress(). The template method pattern delegates subclass-specific behaviour to_do_start(),_do_progress(), and_do_end(). This aligns with PR objectives #635 and #636 for DRY validation and factoring common code.
381-448: LGTM!The
Progressclass correctly definesFIXED = 9for width calculation, adds theno_clearparameter (PR objective #639), callssuper().__init__(), and initialises instance attributes with type hints (PR objective #640).
517-540: LGTM!The
_do_end()method correctly implements theno_clearbehaviour: whenTrue, it preserves the progress bar on the terminal rather than erasing it. This provides users with a visual reference of the completed progress, as per PR objective #639.
632-664: LGTM!The rename from
QuietProgresstoNullProgressaligns with PR objective #645. The class correctly implements the no-op progress pattern, and the unuseddoneparameter in_do_progress()is required to satisfy the abstract method signature.
667-730: LGTM!The
ProgressFactory.get_progress()method correctly adds theno_clearparameter with appropriate documentation, returnsNullProgressfor quiet mode, and passesno_clearonly toProgress(asSimpleProgressdoesn't use terminal control sequences).
733-739: LGTM!The
__all__list correctly exportsNullProgressinstead ofQuietProgress. Note thatProgressBaseis not exported, which is appropriate if it's intended only for internal subclassing. If users should be able to create custom progress implementations, consider addingProgressBaseto the exports.
|
@coderabbitai generate docstrings |
✅ Actions performedInitiated docstring generation; will generate only if new commits exist. |
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @bmr-cymru. * #646 (comment) The following files were modified: * `snapm/_progress.py` * `tests/test_progress.py`
bmr-cymru
force-pushed
the
bmr-progress-improvements
branch
from
2df6cc0 to
3c779fb
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
snapm/_progress.py (1)
131-163: Re-raiseKeyboardInterruptandSystemExitwhen catchingBaseExceptioninTermControlThe current code catches
BaseExceptionaroundcurses.setupterm()to work around curses module quirks, but this also suppressesKeyboardInterruptandSystemExit—both of which inherit directly fromBaseExceptionrather thanException. This defeats normal interrupt handling in CLI tools.Preserve the broad catch for the curses workaround while re-raising these special cases:
try: curses.setupterm() # Attempting to catch curses.error raises 'TypeError: catching classes # that do not inherit from BaseException is not allowed' even though # it claims to inherit from builtins.Exception: # # Help on class error in module _curses: # class error(builtins.Exception) - except BaseException: # pylint: disable=broad-exception-caught + except BaseException as exc: # pylint: disable=broad-exception-caught + # Preserve normal interruption/termination semantics. + if isinstance(exc, (KeyboardInterrupt, SystemExit)): + raise return # pragma: no cover
🧹 Nitpick comments (2)
snapm/_progress.py (2)
232-302:ProgressBase’s width calculation and preconditions look robustThe shared
_calculate_widthand_check_in_progresslogic provides a clean, centralised enforcement of:
FIXEDinitialisation,headerpresence,- mutual exclusivity of
width/width_frac,- sensible defaults based on terminal columns, and
- negative / >total checks.
This should make custom subclasses much harder to misuse. Only minor nit: the
_do_startdocstring still documents atotalparameter even though the method is called without arguments; you might want to update that docstring for accuracy at some point.
647-685:NullProgressimplementation matches the “no output but full validation” goalSubclassing
ProgressBaseand implementing_do_start,_do_progress, and_do_endas pure no-ops is exactly what you want here; all state and argument checks are inherited. One very minor nit is that thedoneargument is unused; if you want to quieten static analysis you could rename it to_doneor add an inline# noqa: ARG002comment, but this is purely cosmetic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snapm/_progress.py(17 hunks)snapm/_snapm.py(2 hunks)tests/test_progress.py(9 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
snapm/_snapm.py (1)
snapm/_progress.py (1)
NullProgress(647-685)
🪛 Ruff (0.14.7)
snapm/_progress.py
162-162: Do not catch blind exception: BaseException
(BLE001)
275-278: Avoid specifying long messages outside the exception class
(TRY003)
281-284: Avoid specifying long messages outside the exception class
(TRY003)
287-287: Avoid specifying long messages outside the exception class
(TRY003)
311-311: Avoid specifying long messages outside the exception class
(TRY003)
340-340: Avoid specifying long messages outside the exception class
(TRY003)
343-343: Avoid specifying long messages outside the exception class
(TRY003)
346-346: Avoid specifying long messages outside the exception class
(TRY003)
456-456: Avoid specifying long messages outside the exception class
(TRY003)
663-663: Unused method argument: done
(ARG002)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (38)
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: python-basic-tests
🔇 Additional comments (13)
snapm/_snapm.py (1)
25-31: Public API rename toNullProgresslooks consistentThe import and
__all__update cleanly switch the public symbol fromQuietProgresstoNullProgressand align with the new implementation and tests. No issues spotted here.Also applies to: 2191-2291
tests/test_progress.py (7)
54-75: TermControl colour initialisation test matches_ANSI_COLORSmappingThe mocked
tigetstr/tparmsetup and thetc.BLACK == "\x1b[30m"assertion correctly exercise the updated colour initialisation logic (BLACK mapped to index 0). The test remains fully decoupled from any real terminal and looks stable.
91-131: Good invariants coverage forProgressBasesubclassesThe
BadProgressinner classes deliberately violatingFIXEDandheaderrequirements give precise, fast feedback if future subclasses misuse_calculate_width(). The regexes match the error prefixes, so small wording tweaks won't break the tests. Nicely targeted coverage.
207-241:no_clearlifecycle behaviour is well specified by this testThe
test_lifecycle_no_clearcase exercises the full start/progress/end flow and asserts that both the header and final message remain visible with exactly two lines of output, which matches the intended semantics forno_clear=True. This is a solid regression test for the new flag.
265-280: State-machine checks forProgressafter end are clear and strictThe tests for
endbeforestartandprogressafterendenforce the strengthened state machine behaviour via the shared error message fragment “called before start”. This aligns with theProgressBase._check_in_progresslogic and should prevent subtle misuse.
298-300:SimpleProgressflow and validation tests look goodThe expected bar string for 50% at width 50 matches the current formatting logic, and the various negative / out-of-range / pre-start / post-end tests comprehensively exercise
SimpleProgress’s shared validation viaProgressBase. Nothing to change here.Also applies to: 315-336
350-400:NullProgresslifecycle tests accurately capture the no-op contractThe tests ensure
NullProgressis:
- silent during a normal lifecycle, and
- still subject to the same argument and state validation as other
ProgressBasesubclasses.That gives good confidence the “quiet” implementation doesn’t accidentally skip validation.
402-435: Factory quiet-path now correctly returnsNullProgress
test_get_progress_quietand the args-validation test together confirm:
quiet=TrueyieldsNullProgress, and- conflicting
width/width_fracarguments are still rejected in the non-quiet path.This neatly locks in the intended factory semantics.
snapm/_progress.py (5)
17-25: Width and colour refactors inTermControlare soundThe introduction of
DEFAULT_COLUMNS,PROGRESS_MIN_WIDTH, andDEFAULT_WIDTH_FRACgives a clear, centralised policy for width calculation. Splitting_COLORSand_ANSI_COLORSand usingenumeratein the colour setup loops correctly aligns indices with the intended terminfo capabilities and matches the updated tests asserting BLACK initialisation. No functional issues spotted here.Also applies to: 121-130, 174-201
303-382: Lifecycle orchestration inProgressBase.start/progress/endis coherentThe pattern of:
- validating
totalinstart(),- using
_check_in_progress()in bothprogress()andend(), and- having
end()drive a finalprogress(self.total, "")before_do_end()gives consistent semantics across all subclasses (including
NullProgress) and cleanly enforces the “no progress before start / after end / negative / >total” rules now covered by the tests. The internal resetting oftotalto0after_do_end()is also aligned with the test expectations.
409-462:Progressintegration withProgressBaseandno_clearbehaves as intendedThe constructor now cleanly:
- delegates common initialisation to
ProgressBase,- sets
header,term,stream,- validates the required terminal capabilities, and
- uses
_calculate_width()for consistent bar sizing while honouring explicitwidth/width_frac.The
no_clearflag is correctly honoured in_do_end(), matching the newtest_lifecycle_no_clearexpectations while preserving the original behaviour whenno_clearisFalse. The Unicode/ASCII fallback for bar characters is unchanged and still safe.No functional problems seen here.
Also applies to: 476-554
561-645:SimpleProgress’s move ontoProgressBaseis straightforward
SimpleProgressnow reuses the shared lifecycle and width logic, while keeping its own formatting (BAR,DID,TODO,FIXED)._do_startbeing a no-op is explicitly documented, and_do_progress/_do_endmatch the expectations in the updated tests. This significantly reduces duplication without changing observable behaviour.
693-752:ProgressFactorychanges correctly wire inNullProgressandno_clearThe factory now:
- returns
NullProgress()whenquietisTrue,- keeps using
SimpleProgressfor non-tty streams, and- passes
no_clearthrough when constructingProgressfor real terminals.This matches both the design goals and the updated tests. Arguments and type hints in the docstring are also up to date.
Also applies to: 755-760
bmr-cymru
force-pushed
the
bmr-progress-improvements
branch
from
3c779fb to
b4ead55
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
snapm/_progress.py (1)
379-389: Minor: Redundant validation inend().The call to
_check_in_progress(self.total)on line 386 validates state, butself.progress(self.total, "")on line 387 will invoke_check_in_progressagain internally. This is harmless but slightly redundant.Consider removing the explicit
_check_in_progresscall and relying on the one insideprogress():def end(self, message: Optional[str] = None): - self._check_in_progress(self.total) + if self.total == 0: + theclass = self.__class__.__name__ + raise ValueError(f"{theclass}.end() called before start()") self.progress(self.total, "") self._do_end(message) self.total = 0Alternatively, keep the current approach for clarity—it explicitly documents that
end()requires an active progress run.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snapm/_progress.py(16 hunks)snapm/_snapm.py(2 hunks)tests/test_progress.py(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- snapm/_snapm.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-22T20:06:54.993Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Applied to files:
snapm/_progress.py
🧬 Code graph analysis (2)
tests/test_progress.py (1)
snapm/_progress.py (23)
ProgressBase(243-398)Progress(401-552)SimpleProgress(555-640)NullProgress(643-678)TermControl(27-240)_do_start(329-335)_do_start(485-490)_do_start(593-599)_do_start(648-654)_do_progress(369-377)_do_progress(492-528)_do_progress(601-627)_do_progress(656-667)_do_end(392-398)_do_end(530-552)_do_end(629-640)_do_end(669-678)progress(356-366)end(379-389)render(221-234)start(314-326)ProgressFactory(681-745)get_progress(687-745)
snapm/_progress.py (1)
tests/test_progress.py (6)
_do_start(101-102)_do_start(120-121)_do_progress(104-105)_do_progress(123-124)_do_end(107-108)_do_end(126-127)
🪛 Ruff (0.14.7)
snapm/_progress.py
286-289: Avoid specifying long messages outside the exception class
(TRY003)
292-295: Avoid specifying long messages outside the exception class
(TRY003)
298-298: Avoid specifying long messages outside the exception class
(TRY003)
322-322: Avoid specifying long messages outside the exception class
(TRY003)
348-348: Avoid specifying long messages outside the exception class
(TRY003)
351-351: Avoid specifying long messages outside the exception class
(TRY003)
354-354: Avoid specifying long messages outside the exception class
(TRY003)
464-464: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (31)
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: python-basic-tests
🔇 Additional comments (22)
tests/test_progress.py (8)
14-21: LGTM!The imports correctly reflect the refactored module structure, adding
ProgressBaseandNullProgresswhilst removing the deprecatedQuietProgress.
62-75: LGTM!The test correctly validates
BLACKcolour initialisation using the mockedtparmreturn value\x1b[30m, fixing the previous incorrect assertion that checkedGREEN. This aligns with issue #643.
92-131: LGTM!The
TestProgressBaseclass provides good coverage for the validation logic in_calculate_width, testing both theFIXEDconstant requirement and theheaderattribute requirement. The subclasses correctly implement the abstract methods to isolate the specific validation being tested.
174-216: LGTM!The lifecycle test correctly validates the new progress flow where
start()initialises state without producing output, and subsequentprogress()calls render the bar. The assertions properly check for terminal control sequences and message content.
218-262: LGTM!Good coverage for the new
no_clear=Trueoption. The test correctly validates that whenno_clearis enabled, the progress bar remains visible afterend()is called, as evidenced by checking for both the header and message in the output along with the two-line structure.
286-300: LGTM!Good additions for state-machine enforcement. These tests validate that
end()beforestart()andprogress()afterend()correctly raiseValueError, ensuring proper lifecycle sequencing as specified in issue #636.
371-420: LGTM!The
TestNullProgressclass correctly validates thatNullProgress(renamed fromQuietProgress) produces no output whilst still enforcing the same lifecycle validation rules inherited fromProgressBase. This ensures consistent behaviour across all progress implementations.
423-426: LGTM!The factory test correctly validates that
quiet=Truenow returns aNullProgressinstance, aligning with the rename fromQuietProgress.snapm/_progress.py (14)
17-24: LGTM!Well-documented module-level constants that centralise previously scattered magic numbers. The defaults are sensible and improve maintainability.
128-131: LGTM!Correctly separates legacy (
setf) and ANSI (setaf) colour orderings. The ANSI order follows the standard SGR colour sequence, fixing issue #642.
133-162: LGTM!The
_init_colorsmethod is well-structured, usingenumerate()as per issue #638, and correctly handles both legacy terminfo (setf/setb) and ANSI (setaf/setab) colour capabilities. The separation of foreground and background initialisation is clear and maintainable.
187-199: LGTM!Correctly catches
BaseExceptionto handle thecurses.errorissue documented in the comment (issue #644). Importantly, the code re-raisesKeyboardInterruptandSystemExitto preserve normal termination semantics.
243-261: LGTM!The
ProgressBaseABC provides a clean template method pattern with well-typed instance attributes. TheFIXED = -1sentinel ensures subclasses must override it before width calculation.
262-312: LGTM!The
_calculate_widthmethod correctly validates prerequisites and calculates the progress bar width. The logic for falling back toDEFAULT_COLUMNSwhen terminal width is unavailable, and enforcingPROGRESS_MIN_WIDTHas a minimum, are sensible safeguards.
314-326: LGTM!The
startmethod correctly validatestotal > 0, sets state, and delegates to the_do_starthook. This centralises the validation logic as required by issue #636.
337-354: LGTM!The
_check_in_progressmethod correctly validates progress state with clear, class-specific error messages. Using the class name in messages aids debugging for subclass-specific issues.
424-469: LGTM!The
Progressclass correctly integrates theno_clearoption and uses thefirst_updateflag to handle cursor positioning differently for initial vs subsequent progress updates. Thesuper().__init__()call properly chains toProgressBase.
530-552: LGTM!The
_do_endmethod correctly implements theno_clearoption. Whenno_clear=False, the progress bar is erased; whenno_clear=True, the bar remains visible and only cursor positioning is adjusted before printing the completion message.
555-641: LGTM!
SimpleProgresscorrectly integrates withProgressBase, definingFIXED = 12for its format string and implementing the required hooks. The no-op_do_startis appropriate since there's no terminal setup needed for simple line-by-line output.
643-679: LGTM!
NullProgresscorrectly inherits fromProgressBaseand implements all hooks as no-ops. Since it produces no output, it doesn't need to overrideFIXEDor calculate width, making the inherited defaults appropriate.
686-745: LGTM!The factory correctly routes to
NullProgresswhenquiet=True,SimpleProgressfor non-TTY streams, andProgresswith terminal capabilities otherwise. Theno_clearparameter is properly passed through toProgress.
748-754: LGTM!The
__all__list correctly exports the public API. Note thatProgressBaseis not exported (which is appropriate for an ABC intended for internal inheritance), though it can still be imported for testing purposes.
…nd() Use template `start()`, `stop()`, `end()` methods in `ProgressBase` and delegate to concrete classes via `self._do_*()` hooks. Resolves: #636 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #637 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #638 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #640 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #641 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
bmr-cymru
force-pushed
the
bmr-progress-improvements
branch
from
b4ead55 to
b27af6f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/test_progress.py (1)
218-262: no_clear behaviour is well covered; minor optional strengtheningThe
test_lifecycle_no_clearexercise ofno_clear=Trueand the check for two output lines containing the header and final message gives good confidence that the last bar is preserved. If you ever want to tighten this further, you could also assert that the final output contains": 100% ["to confirm the completed-bar rendering without making the test overly brittle.snapm/_progress.py (2)
416-469: Progress initialisation cleanly uses the base class and new optionsUsing
super().__init__()plus explicitheader,term,stream, and width calculation keeps setup straightforward. The capabilities check onCLEAR_EOL,UP, andBOLis a reasonable minimal contract, and theno_clearflag andfirst_updatestate integrate neatly with the hook methods. One small follow-up: the__init__docstring doesn’t yet describe theno_clearparameter, so adding a:param no_clear:entry (similar toProgressFactory.get_progress) would keep the public API documentation consistent.
285-355: TRY003 warnings are style-only; current messages are acceptableRuff’s TRY003 hints here are about long
ValueErrormessages being defined inline rather than on custom exception types. Given these errors are user-facing and already quite clear, I’d treat this as a style choice rather than something to change immediately; if you prefer a quieter linter, you could either suppress TRY003 for this module or gradually refactor to custom exception classes later.Also applies to: 463-463
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snapm/_progress.py(16 hunks)snapm/_snapm.py(2 hunks)tests/test_progress.py(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- snapm/_snapm.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
📚 Learning: 2025-10-22T20:06:54.993Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Applied to files:
snapm/_progress.py
🪛 Ruff (0.14.7)
snapm/_progress.py
286-289: Avoid specifying long messages outside the exception class
(TRY003)
292-295: Avoid specifying long messages outside the exception class
(TRY003)
298-298: Avoid specifying long messages outside the exception class
(TRY003)
322-322: Avoid specifying long messages outside the exception class
(TRY003)
348-348: Avoid specifying long messages outside the exception class
(TRY003)
351-351: Avoid specifying long messages outside the exception class
(TRY003)
354-354: Avoid specifying long messages outside the exception class
(TRY003)
463-463: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (38)
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: python-basic-tests
🔇 Additional comments (21)
tests/test_progress.py (8)
58-76: TermControl colour capability test matches new initialisation logicThe updated mocking of
tigetstr/tparmand the assertion againsttc.BLACKcorrectly exercise the legacy colour path and ensure index 0 maps to black, which aligns with the new_ANSI_COLORS/_LEGACY_COLORShandling.
92-131: Good coverage of ProgressBase width preconditionsThe
BadProgresssubclasses nicely pin down the requirements thatFIXEDandheaderbe initialised before_calculate_width()is used. The regexes are tolerant of minor message wording changes while still checking the key phrases.
174-217: Lifecycle test updated correctly for deferred initial renderThe
test_lifecycleassertions for no output onstart(), presence of header and message onprogress(), andtotalreset to 0 afterend()all match the newProgressBase/Progressbehaviour of deferring the first draw until the first update and clearing state on completion.
286-307: State-machine tests for Progress align with ProgressBase checksThe new
test_end_before_start_raisesandtest_progress_after_end_raisescorrectly validate the sharedProgressBase._check_in_progress()semantics:end()beforestart()andprogress()afterend()both surface as “called before start”. This is a useful regression guard for the lifecycle logic.
319-341: SimpleProgress expectations and error handling look correctThe updated
test_flowstring expectation matches the new width calculation and bar formatting, and the negative-progress test now simply asserts aValueError, which is consistent with the shared base-class validation.
342-369: SimpleProgress inherits the same lifecycle guarantees as ProgressThe new tests for
end()beforestart(),progress()afterend(), anddone > totalall exercise theProgressBasevalidation path for the simple implementation, ensuring consistent semantics across both progress styles.
371-421: NullProgress tests correctly validate no-op behaviour plus state checksThe
TestNullProgresssuite verifies that lifecycle methods are callable without output while still enforcingtotal > 0, non-negative progress,done <= total, and the same “called before start” behaviour forend()/post-end()progress. This is exactly what you want from a quiet/no-op implementation.
423-456: Factory now correctly returns NullProgress for quiet modeAsserting that
get_progress(..., quiet=True)yields aNullProgressinstance keeps the tests aligned with the public API rename fromQuietProgressand protects the quiet-path behaviour.snapm/_progress.py (13)
17-25: Centralised width defaults are clear and reusableIntroducing
DEFAULT_COLUMNS,PROGRESS_MIN_WIDTH, andDEFAULT_WIDTH_FRACgives a single source of truth for sizing across implementations and makes tests easier to reason about. Values chosen are sensible for typical terminals.
128-163: Colour initialisation correctly separates legacy and ANSI mappingsThe
_LEGACY_COLORSvs_ANSI_COLORSsplit, combined with_init_colors(), matches the expectedsetf/setbvssetaf/setaborders and ensures ANSI indices no longer get the wrong colour names. The use of_tigetstr()keeps capability handling consistent and the fallback behaviour (empty strings when capabilities are absent) remains intact.
164-211: TermControl initialisation and error handling align with curses quirksThe updated
__init__correctly bails out for non-ttys, usesBaseExceptionto sidestep the oddcurses.errorinheritance issue while still re-raisingKeyboardInterrupt/SystemExit, and then delegates colour setup to_init_colors(). This matches the intent of the linked issues and behaves safely when curses capabilities are missing.
221-235: render() docstring now fully describes substitution behaviourThe expanded docstring clearly documents
${NAME}handling, the empty-string fallback for unknown capabilities, and the$$escape, which matches the implementation and helps callers understand how to embed control sequences in templates.
243-313: ProgressBase initial state and width calculation look soundThe base
__init__initialises all shared attributes, and_calculate_width()enforcesFIXEDandheaderpreconditions, mutual exclusivity ofwidth/width_frac, and applies sensible defaults (DEFAULT_COLUMNS,DEFAULT_WIDTH_FRAC,PROGRESS_MIN_WIDTH). The use ofself.FIXED + len(self.header)to derive available bar width matches the tests and keeps derived classes consistent.
314-389: Shared lifecycle and range checks in ProgressBase are well factored
start(),_check_in_progress(),progress(), andend()now centralise validation and state transitions (including zeroingtotalafter completion). The error messages are clear and the tests exercise all key paths (non-positive totals, negative/overshoot progress, before-start and after-end calls), delivering the DRY/state-machine guarantees the PR aims for.
484-531: Deferred first render and update prefix logic are consistentInitialising
pbarin_do_start()without emitting output, and then usingfirst_updatein_do_progress()to switch from a simple${BOL}prefix on the first write to${BOL}${UP}${CLEAR_EOL}for subsequent updates, matches the tests’ expectations and avoids the extra initial bar draw that SimpleProgress never had. The flush-on-write behaviour is preserved.
532-555: no_clear end-of-progress behaviour matches tests and intentThe
_do_end()branch forno_clear=Falseclears both lines of the bar, while theno_clear=Truepath leaves the completed bar in place and only tidies the current line before printing the optional message. This matches the new tests (including line-count expectations) and fulfils the “leave the bar visible” requirement.
562-594: SimpleProgress correctly adopts ProgressBase while remaining tty-agnosticThe new constants (
BAR,DID,TODO,FIXED) and the use of_calculate_width()in__init__keep behaviour in line withProgress, while_do_start()remaining a no-op preserves the simple, single-line semantics. The stream typing and assignment (term_stream or sys.stdout) look correct and resolve the earlier type-hint concern.
603-643: SimpleProgress update and end hooks remain straightforward and correct
_do_progress()now relies on the shared validation fromProgressBaseand keeps its original formatting and flushing behaviour, and_do_end()simply prints the optional message then flushes. This minimalism is appropriate for the non-curses implementation and matches the updated tests.
645-681: NullProgress cleanly implements a no-op ProgressBase subclassBy implementing the three hook methods as no-ops and marking parameters as unused,
NullProgressleverages all lifecycle and validation logic fromProgressBasewhile guaranteeing silent operation. This nicely replaces the oldQuietProgressand aligns with the new tests that assert identical state-machine semantics without output.
688-747: ProgressFactory wiring and documentation look consistent with the new API
get_progress()now returnsNullProgressforquiet=True, falls back toSimpleProgressfor non-tty streams, and constructsProgressfor tty streams while passing throughwidth,width_frac, andno_clear(plus optionalterm_control). The expanded docstring forno_clearclearly explains its effect and notes that it’s ignored by implementations that don’t useTermControl.
750-757: Public export surface matches the refactored progress APIUpdating
__all__to includeNullProgressandProgressBase(and drop the oldQuietProgress) keeps the module’s public surface aligned with the new class hierarchy and the tests/imports insnapm._snapmandtests/test_progress.py.
Resolves: #643 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #644 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #645 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #648 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #649 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #650 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #651 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
Resolves: #652 Signed-off-by: Bryn M. Reeves <bmr@redhat.com>
bmr-cymru
force-pushed
the
bmr-progress-improvements
branch
from
b27af6f to
7439b1f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
snapm/_progress.py (1)
286-289: Consider shorter exception messages (optional).Static analysis flags these
ValueErrormessages as lengthy. Whilst the descriptive messages aid debugging, you might consider defining custom exception classes or shortening messages if you prefer stricter adherence to style guides.Also applies to: 292-295, 298-298, 322-322, 348-348, 351-351, 354-354, 470-470
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
snapm/_progress.py(16 hunks)snapm/_snapm.py(2 hunks)tests/test_progress.py(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/test_progress.py
- snapm/_snapm.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
📚 Learning: 2025-10-22T20:06:54.993Z
Learnt from: bmr-cymru
Repo: snapshotmanager/snapm PR: 541
File: snapm/manager/_mounts.py:174-197
Timestamp: 2025-10-22T20:06:54.993Z
Learning: For snapshotmanager/snapm follow-up requests from bmr-cymru, create a GitHub issue with label "enhancement", set/ensure a "type: Task" label, and assign it to bmr-cymru. Include backlinks to the PR and the discussion comment in the issue body.
Applied to files:
snapm/_progress.py
🧬 Code graph analysis (1)
snapm/_progress.py (1)
tests/test_progress.py (6)
_do_start(101-102)_do_start(120-121)_do_progress(104-105)_do_progress(123-124)_do_end(107-108)_do_end(126-127)
🪛 Ruff (0.14.7)
snapm/_progress.py
286-289: Avoid specifying long messages outside the exception class
(TRY003)
292-295: Avoid specifying long messages outside the exception class
(TRY003)
298-298: Avoid specifying long messages outside the exception class
(TRY003)
322-322: Avoid specifying long messages outside the exception class
(TRY003)
348-348: Avoid specifying long messages outside the exception class
(TRY003)
351-351: Avoid specifying long messages outside the exception class
(TRY003)
354-354: Avoid specifying long messages outside the exception class
(TRY003)
470-470: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (33)
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: virt_tests (bios, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (bios, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora41)
- GitHub Check: virt_tests (bios, lvm, fedora41)
- GitHub Check: virt_tests (uefi, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora42)
- GitHub Check: virt_tests (uefi, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream10)
- GitHub Check: virt_tests (bios, lvm-thin, centos-stream9)
- GitHub Check: virt_tests (uefi, lvm-thin, fedora41)
- GitHub Check: virt_tests (uefi, lvm, centos-stream10)
- GitHub Check: virt_tests (bios, lvm, centos-stream10)
- GitHub Check: virt_tests (uefi, lvm, centos-stream9)
- GitHub Check: virt_tests (bios, lvm, fedora42)
- GitHub Check: virt_tests (bios, lvm, centos-stream9)
- GitHub Check: python-basic-tests
- GitHub Check: container-tests
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:fedora-41-x86_64
- GitHub Check: rpm-build:epel-9-x86_64
- GitHub Check: rpm-build:centos-stream-9-x86_64
- GitHub Check: rpm-build:fedora-42-x86_64
- GitHub Check: rpm-build:fedora-rawhide-x86_64
- GitHub Check: rpm-build:fedora-43-x86_64
- GitHub Check: rpm-build:centos-stream-10-x86_64
🔇 Additional comments (12)
snapm/_progress.py (12)
17-24: Well-defined module constants.These constants support centralised width calculation and provide sensible defaults. The documentation is clear and concise.
128-162: Excellent factoring of colour initialisation.The new
_init_colorsmethod cleanly separates legacy and ANSI colour setup, usesenumerate()for cleaner iteration, and correctly applies_ANSI_COLORSfor ANSI foreground colours (fixing issue #642).
195-211: Robust exception handling preserves system exit semantics.Catching
BaseExceptionhandles curses-related errors that don't inherit fromException, whilst the re-raise logic correctly preservesKeyboardInterruptandSystemExitto maintain normal interruption semantics.
243-312: Solid abstraction with template method pattern.The
ProgressBaseclass effectively centralises validation and width calculation whilst delegating rendering to subclasses. The_calculate_widthmethod is thorough with appropriate validation, and the FIXED sentinel value enforces proper subclass configuration.
314-397: Clean lifecycle hooks with centralised validation.The template method pattern correctly separates concerns: public methods handle validation and state transitions whilst abstract
_do_*hooks implement subclass-specific behaviour. The_check_in_progressmethod effectively centralises validation logic.
423-476: Well-implementedno_clearfeature.The
no_clearparameter allows users to preserve the final progress display. Type hints are correctly applied, and the width calculation properly delegates to the base class method.
491-537: Cleverfirst_updatelogic avoids initial bar output.The conditional prefix handling ensures the first progress update uses only
BOL, whilst subsequent updates useBOL + UP + CLEAR_EOLto overwrite the previous display. This matches the behaviour ofSimpleProgressand addresses objective #651.
539-561: Correctno_clearend behaviour for two-line display.When
no_clear=False, both lines are properly cleared. Whenno_clear=True, the progress bar (line 1) is preserved, the message line (line 2) is cleared and repositioned, and an optional completion message can be printed below the progress bar.
564-650:SimpleProgresscorrectly adapted to new base class.The class properly implements the
ProgressBasehooks with appropriateFIXEDvalue, type hints, and width calculation delegation. The implementation remains straightforward for non-TTY streams.
652-688: Excellent rename toNullProgress.The rename from
QuietProgresstoNullProgressbetter conveys the "no-op" behaviour. Unused parameters are correctly marked with underscore prefixes, andpragma: no coverappropriately excludes these trivial no-ops from coverage.
696-754: Factory correctly updated for new API.The
no_clearparameter is properly threaded through to theProgressclass, documentation clearly notes it's ignored by non-terminal implementations, and the factory correctly returnsNullProgressfor quiet mode whilst maintaining backward compatibility.
757-764: Public API correctly updated.Exports properly reflect the refactoring:
NullProgressreplacesQuietProgress,ProgressBaseis exposed for custom implementations, and existing classes are maintained. Module constants remain unexported, which appears intentional for internal use.
|
@coderabbitai: re:
Not for me it doesn't: Anyway, they aren't long. Most are fixed strings, and the few that aren't include the class name for good reason (debugging API violating subclasses). It's not worth defining a new |
|
Tip For best results, initiate chat on the files or code changes. @bmr-cymru, understood! Thank you for the clarification. If Ruff passes cleanly for you and the messages serve a clear debugging purpose, that's perfectly reasonable. I appreciate you taking the time to explain the rationale—your approach makes sense given the context. 🧠 Learnings used |
Resolves: #635
Resolves: #636
Resolves: #637
Resolves: #638
Resolves: #639
Resolves: #640
Resolves: #641
Resolves: #642
Resolves: #643
Resolves: #644
Resolves: #645
Resolves: #648
Resolves: #649
Resolves: #650
Resolves: #651
Resolves: #652
Resolves: #653
Summary by CodeRabbit
New Features
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.