Conversation
There was a problem hiding this comment.
Pull request overview
Adds basic “units” support to progress bars so callers can label the displayed progress/total values (e.g., 2/3 syllables), as a lightweight bridge until the more comprehensive progress bar work in #426 lands.
Changes:
- Add
unitssupport toEmitter.progress_bar(...)and thread it through_Progresser→Printer.progress_bar(...). - Render the units label in terminal progress bar output (
... 2/3 <units>). - Update/extend unit tests, examples, and changelog to cover/document the new parameter.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
craft_cli/printer.py |
Stores units on bar messages and appends them to the numeric progress display in terminal output. |
craft_cli/messages.py |
Adds units to the Emitter.progress_bar API and propagates it through _Progresser. |
tests/unit/test_printer.py |
Adds a unit test asserting units are displayed in terminal progress bars; updates existing progress_bar calls for new signature. |
tests/unit/test_messages_helpers.py |
Updates _Progresser tests to pass through and assert units=None. |
examples.py |
Demonstrates progress bars with units in an example. |
docs/changelog.rst |
Documents the new units parameter under the next release entry. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
craft_cli/printer.py
Outdated
| progress: float, | ||
| total: float, | ||
| use_timestamp: bool, | ||
| units: str | None, |
There was a problem hiding this comment.
Printer.progress_bar() now requires the units keyword argument. Even though Printer isn’t exported from craft_cli.__init__, it is still a concrete class in a public module and this is an avoidable breaking change for any direct users/tests. Consider giving units a default of None (and keeping it keyword-only) so existing call sites continue to work without modification.
| units: str | None, | |
| units: str | None = None, |
There was a problem hiding this comment.
I don't mind the change since it is public in a strict sense. But I'm pretty sure it's entirely private in practice.
medubelko
left a comment
There was a problem hiding this comment.
Looks neat!
Probably an out-of-scope question, but when the unit string is very long, all progress is clipped:
boop
boop
Crunching the numbersCould we still show the numbers when we clip the units?
boop
boop
Crunching the numbers [59/60]
make lint && make test?Adds support for units in progress bars.
Since more sophisticated progress bars are coming from #426, this is an intentionally simple implementation to accommodate other work until the other PR can be released.
CRAFT-5029