Skip to content

feat: support units for progress bars#428

Open
bepri wants to merge 6 commits intomainfrom
work/progress-bar-units/CRAFT-5029
Open

feat: support units for progress bars#428
bepri wants to merge 6 commits intomainfrom
work/progress-bar-units/CRAFT-5029

Conversation

@bepri
Copy link
Member

@bepri bepri commented Mar 20, 2026

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • Have you successfully run 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

@bepri bepri requested a review from lengau March 20, 2026 20:29
@bepri bepri self-assigned this Mar 20, 2026
@bepri bepri requested review from a team, medubelko and tigarmo as code owners March 20, 2026 20:29
@bepri bepri removed request for a team and medubelko March 20, 2026 20:29
@bepri bepri marked this pull request as draft March 20, 2026 20:32
@bepri bepri marked this pull request as ready for review March 20, 2026 20:40
@bepri bepri requested review from cmatsuoka and removed request for tigarmo March 24, 2026 13:59
Copy link
Contributor

@cmatsuoka cmatsuoka left a comment

Choose a reason for hiding this comment

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

Looks good

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

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 units support to Emitter.progress_bar(...) and thread it through _ProgresserPrinter.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.

progress: float,
total: float,
use_timestamp: bool,
units: str | None,
Copy link

Copilot AI Mar 24, 2026

Choose a reason for hiding this comment

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

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.

Suggested change
units: str | None,
units: str | None = None,

Copilot uses AI. Check for mistakes.
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

@medubelko medubelko left a comment

Choose a reason for hiding this comment

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

Looks neat!

Probably an out-of-scope question, but when the unit string is very long, all progress is clipped:

boop
boop
Crunching the numbers

Could we still show the numbers when we clip the units?

boop
boop
Crunching the numbers [59/60]

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.

4 participants